-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
Conversation
994779a
to
a89f98c
Compare
a89f98c
to
a16fe99
Compare
@@ -9,7 +9,11 @@ import FormLabel from '@mui/material/FormLabel'; | |||
import FormHelperText from '@mui/material/FormHelperText'; | |||
import InputBase, { inputBaseClasses } from '@mui/material/InputBase'; | |||
import CheckCircleRoundedIcon from '@mui/icons-material/CheckCircleRounded'; | |||
import CONFIG from 'docs/src/config'; | |||
|
|||
const NEWSLETTER_SUBSCRIBE_URL = |
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?
env: {
NEWSLETTER_SUBSCRIBE_URL: isProduction ? '...' : '...',
GA_ID: isProduction ? 'UA-106598593-2' : 'UA-106598593-3',
GA_SAMPLING_RATE: isProduction ? 80 : 100,
ENABLE_TOOLPAD: isProduction,
// ...
}
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
@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.
What's the value of this change?
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.
// Minifies to `const NEWSLETTER_SUBSCRIBE_URL = 'url a'` const NEWSLETTER_SUBSCRIBE_URL = process.env.DEPLOY_ENV === 'production' ? 'url a' : 'url b' // Minifies to // const isProd = () => true // const NEWSLETTER_SUBSCRIBE_URL = isProd() ? 'url a' : 'url b' const isProd = () => process.env.DEPLOY_ENV === 'production' const NEWSLETTER_SUBSCRIBE_URL = isProd() ? 'url a' : 'url b' // Injecting process.env.NEWSLETTER_SUBSCRIBE_URL directly prevents this from ever happening
-
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
I would also be in favor of removing
docs/src/route
. The values feel no different from using the URLs directly.
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 a sectionLink('intro')
everywhere that could be easily listed using "find references" in vscode
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 minor, I'm more than fine for this PR to be merged as is
It has been a week, I'm merging. |
The current configuration leads to this bug:
So instead, I have created a function to extend a Next.js configuration file. It's similar to https://github.com/martpie/next-transpile-modules/blob/master/src/next-transpile-modules.js. It was suggested in mui/toolpad#866 (comment). I have used the opportunity to normalize how we use env tokens based on mui/toolpad#809 (comment).
https://deploy-preview-34259--material-ui.netlify.app/
This change should unlock: