Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[core] Create shared Next.js baseline config #34259
[core] Create shared Next.js baseline config #34259
Changes from 3 commits
874b9a3
a16fe99
267217a
2cbf834
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to avoid passing
DEPLOY_ENV
to the application code entirely, and instead calculate these options centrally, in the next.js baseline config?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Janpot What's the value of this change? I understand the value of this indirection for secrets and constants used multiple times, but I don't get it for the rest. My goal in removing
docs/src/config
was to centralize the logic as much as possible in the same files.A side note. I would also be in favor of removing
docs/src/route
. The values feel no different from using the URLs directly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not critical at all, it would mostly improve maintainability a bit and be more defensive code.
It creates a natural overview of which features are enabled for which environment. This is especially handy when a new environment has to be defined (e.g. new type of integration tests)
Injects the environment specific flags through the webpack define plugin. This prevents future maintainers from accidentally creating code that doesn't minify unused configuration away. e.g.
Avoids duplicating logic in case a certain flag is used in multiple places in the application. Or when the same flag is used between build time and run time
Yes, it's not critical neither. For me, the main advantage of such a file would be around static analyzability of where certain links are used. Especially when people start defining links like
/section-${sections.slug}
in one place and/section-intro
in others. Such constructs are hard to search for. If you use something like asectionLink('intro')
everywhere that could be easily listed using "find references" in vscodeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's minor, I'm more than fine for this PR to be merged as is
This file was deleted.