Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Ensure app config variables have values at build-time #1296

Merged
merged 5 commits into from
Jan 20, 2023

Conversation

art-alexeyenko
Copy link
Contributor

@art-alexeyenko art-alexeyenko commented Jan 18, 2023

This PR slightly reworks how a config in nextjs app is built.
Each value is resolved with the following logic:

  1. Attempt to retrieve value from env
  2. Attempt to retrieve value from supplementary files (package.json / scjssconfig)
  3. If certain values (that can't be empty, i.e. language) are still empty - use fallback value.

This roughly follows pre-plugin approach to building config where env values take precedence, other sources follow as fallback.
This also ensures that graphQL endpoint will have a correct value, if hostname values is present in either env or scjssconfig.

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)

@art-alexeyenko art-alexeyenko requested a review from a team January 18, 2023 17:37
Copy link
Contributor

@ambrauer ambrauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The computed prop and computeConfigValue required to evaluate the string seems unnecessary. I think you're onto something with the fallback to env values in scjssconfig.ts...

Could we just make this a first step to populate all of the config defaults?
For example, in generate-config.ts, something like:

const defaultConfig: JssConfig = {
  sitecoreApiKey: process.env[`${constantCase('sitecoreApiKey')}`] || 'no-api-key-set',
  sitecoreApiHost: process.env[`${constantCase('sitecoreApiHost')}`] || '',
  jssAppName: process.env[`${constantCase('jssAppName')}`] || 'Unknown',
  graphQLEndpointPath: process.env[`${constantCase('graphQLEndpointPath')}`] || '',
  defaultLanguage: process.env[`${constantCase('defaultLanguage')}`] || 'en',
};

Or maybe even better, a default config plugin that does this? Could probably then lose the defaultConfig altogether in generate-config.ts.

@art-alexeyenko art-alexeyenko changed the title Ensure graphq endpoint can still have a dynamic value in config while being usable at build time with multisite Ensure app config variables have values at build-time Jan 19, 2023
@art-alexeyenko
Copy link
Contributor Author

Reworked the approach, moving away from dynamic evaluation.

Copy link
Contributor

@ambrauer ambrauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, see a few comments

Copy link
Contributor

@ambrauer ambrauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@ambrauer ambrauer merged commit 4b9b0ea into dev Jan 20, 2023
@ambrauer ambrauer deleted the bug/graphql-endpoint-not-dynamic branch January 20, 2023 16:28
@ambrauer
Copy link
Contributor

Resolves #1291

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

Successfully merging this pull request may close these issues.

None yet

2 participants