Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

refactored env vars into new constants file #462

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bklaing2
Copy link
Member

Purpose

There is a common pattern repeated throughout the code when using some ENV vars. For example:

const pageUrl =
    process.env.NEXT_PUBLIC_API_URL === 'https://api.datacite.org'
      ? 'https://commons.datacite.org/doi.org/' + data.work.doi
      : 'https://commons.stage.datacite.org/doi.org/' + data.work.doi

closes: Add github issue that originated this PR

Approach

This PR refactors most environment variables into a constants.ts file. The updated example is now:

const pageUrl = `${COMMONS_URL}/doi.org/${data.work.doi}`

Open Questions and Pre-Merge TODOs

  • Many of the variables in constants.ts rely on checking if process.env.NEXT_PUBLIC_API_URL === 'https://api.datacite.org' to determine if they should use the prod or stage version of their URL. We may consider using the NODE_ENV variable instead.
  • Building on the previous point, most of our ENV variables aren't actually secrets and are already hardcoded as fallbacks anyways. If we use the NODE_ENV to determine which URL to point to then we can remove most of the ENV vars.

Learning

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

  • New feature (non-breaking change which adds functionality)

  • Breaking change (fix or feature that would cause existing functionality to change)

Reviewer, please remember our guidelines:

  • Be humble in the language and feedback you give, ask don't tell.
  • Consider using positive language as opposed to neutral when offering feedback. This is to avoid the negative bias that can occur with neutral language appearing negative.
  • Offer suggestions on how to improve code e.g. simplification or expanding clarity.
  • Ensure you give reasons for the changes you are proposing.

@bklaing2 bklaing2 self-assigned this Feb 22, 2025
Copy link

cypress bot commented Feb 22, 2025

akita    Run #1575

Run Properties:  status check passed Passed #1575  •  git commit ad0e5898e3 ℹ️: Merge 1348c022b9a07756dd8522cc793e9599baac99d0 into 48e76ac7ce8e31f87d140de2c51e...
Project akita
Branch Review refactor-env-vars
Run status status check passed Passed #1575
Run duration 01m 33s
Commit git commit ad0e5898e3 ℹ️: Merge 1348c022b9a07756dd8522cc793e9599baac99d0 into 48e76ac7ce8e31f87d140de2c51e...
Committer Bryceson Laing
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 3
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 50
View all changes introduced in this branch ↗︎

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant