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.

Sign up for GitHub

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

Jump to bottom

[core] Create shared Next.js baseline config #34259

Merged
merged 4 commits into from Sep 17, 2022
Merged

[core] Create shared Next.js baseline config #34259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
874b9a3
[core] Create shared Next.js baseline config
oliviertassinari Sep 11, 2022
a16fe99
fix ci
oliviertassinari Sep 11, 2022
267217a
prevent variable overrides
oliviertassinari Sep 11, 2022
2cbf834
Jan's review
oliviertassinari Sep 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
53 changes: 6 additions & 47 deletions docs/next.config.js
View file
Open in desktop
@@ -1,51 +1,19 @@
const path = require('path');
const { BundleAnalyzerPlugin } = require('webpack-bundle-analyzer');
const pkg = require('../package.json');
const withDocsInfra = require('./nextConfigDocsInfra');
const { findPages } = require('./src/modules/utils/find');
const { LANGUAGES, LANGUAGES_SSR, LANGUAGES_IGNORE_PAGES } = require('./src/modules/constants');

const workspaceRoot = path.join(__dirname, '../');

const reactStrictMode = true;
if (reactStrictMode) {
// eslint-disable-next-line no-console
console.log(`Using React.StrictMode.`);
}
const l10nPRInNetlify = /^l10n_/.test(process.env.HEAD) && process.env.NETLIFY === 'true';
const vercelDeploy = Boolean(process.env.VERCEL);
const isDeployPreview = process.env.PULL_REQUEST === 'true';
const isDeployPreview = Boolean(process.env.PULL_REQUEST_ID);
// For crowdin PRs we want to build all locales for testing.
const buildOnlyEnglishLocale = isDeployPreview && !l10nPRInNetlify && !vercelDeploy;

const staging =
process.env.REPOSITORY_URL === undefined ||
// The linked repository url comes from https://app.netlify.com/sites/material-ui/settings/deploys
/mui-org\/material-ui$/.test(process.env.REPOSITORY_URL);

if (staging) {
// eslint-disable-next-line no-console
console.log(`Staging deploy of ${process.env.REPOSITORY_URL || 'local repository'}`);
}

const baseline = {
experimental: {
scrollRestoration: true,
},
eslint: {
ignoreDuringBuilds: true,
},
typescript: {
// Motivated by https://github.com/vercel/next.js/issues/7687
ignoreBuildErrors: true,
},
trailingSlash: true,
// Can be turned on when https://github.com/vercel/next.js/issues/24640 is fixed
optimizeFonts: false,
};

module.exports = {
...baseline,
baseline, // Exported so the other projects can use this configuration directly.
module.exports = withDocsInfra({
webpack: (config, options) => {
const plugins = config.plugins.slice();

Expand Down Expand Up @@ -182,19 +150,11 @@ module.exports = {
};
},
env: {
COMMIT_REF: process.env.COMMIT_REF,
ENABLE_AD_IN_DEV_MODE: process.env.ENABLE_AD_IN_DEV_MODE,
GITHUB_AUTH: process.env.GITHUB_AUTH,
GIT_REVIEW_ID: process.env.REVIEW_ID,
LIB_VERSION: pkg.version,
NETLIFY_DEPLOY_URL: process.env.DEPLOY_URL || 'http://localhost:3000',
NETLIFY_SITE_NAME: process.env.SITE_NAME || 'material-ui',
PULL_REQUEST: process.env.PULL_REQUEST === 'true',
FEEDBACK_URL: process.env.FEEDBACK_URL,
// #default-branch-switch
SOURCE_CODE_ROOT_URL: 'https://github.com/mui/material-ui/blob/master',
SOURCE_CODE_ROOT_URL: 'https://github.com/mui/material-ui/blob/master', // #default-branch-switch
SOURCE_CODE_REPO: 'https://github.com/mui/material-ui',
STAGING: staging,
BUILD_ONLY_ENGLISH_LOCALE: buildOnlyEnglishLocale,
},
// Next.js provides a `defaultPathMap` argument, we could simplify the logic.
Expand All @@ -207,7 +167,7 @@ module.exports = {
const prefix = userLanguage === 'en' ? '' : `/${userLanguage}`;

pages2.forEach((page) => {
if (page.pathname.startsWith('/experiments') && !staging) {
if (page.pathname.startsWith('/experiments') && process.env.DEPLOY_ENV !== 'production') {
return;
}
// The blog is not translated
Expand Down Expand Up @@ -247,7 +207,6 @@ module.exports = {

return map;
},
reactStrictMode,
// rewrites has no effect when run `next export` for production
rewrites: async () => {
return [
Expand All @@ -257,4 +216,4 @@ module.exports = {
{ source: `/static/x/:rest*`, destination: 'http://0.0.0.0:3001/static/x/:rest*' },
];
},
};
});
62 changes: 62 additions & 0 deletions docs/nextConfigDocsInfra.js
View file
Open in desktop
@@ -0,0 +1,62 @@
let DEPLOY_ENV = 'development';

// Same as process.env.PULL_REQUEST_ID
if (process.env.CONTEXT === 'deploy-preview') {
DEPLOY_ENV = 'pull-request';
}

if (process.env.CONTEXT === 'production' || process.env.CONTEXT === 'branch-deploy') {
DEPLOY_ENV = 'production';
}

if (
process.env.CONTEXT === 'branch-deploy' &&
(process.env.HEAD === 'master' || process.env.HEAD === 'next')
) {
DEPLOY_ENV = 'staging';
}

process.env.DEPLOY_ENV = DEPLOY_ENV;

function withDocsInfra(nextConfig) {
return {
trailingSlash: true,
// Can be turned on when https://github.com/vercel/next.js/issues/24640 is fixed
optimizeFonts: false,
reactStrictMode: true,
...nextConfig,
env: {
BUILD_ONLY_ENGLISH_LOCALE: true, // disable translations by default
// production | staging | pull-request | development
DEPLOY_ENV,
...nextConfig.env,
// https://docs.netlify.com/configure-builds/environment-variables/#git-metadata
// reference ID (also known as “SHA” or “hash”) of the commit we’re building.
COMMIT_REF: process.env.COMMIT_REF,
// ID of the PR and the Deploy Preview it generated (for example, 1211)
PULL_REQUEST_ID: process.env.REVIEW_ID,
// This can be set manually in the .env to see the ads in dev mode.
ENABLE_AD_IN_DEV_MODE: process.env.ENABLE_AD_IN_DEV_MODE,
// URL representing the unique URL for an individual deploy, e.g.
// https://5b243e66dd6a547b4fee73ae--petsof.netlify.app
NETLIFY_DEPLOY_URL: process.env.DEPLOY_URL,
// Name of the site, its Netlify subdomain; for example, material-ui-docs
NETLIFY_SITE_NAME: process.env.SITE_NAME,
},
experimental: {
scrollRestoration: true,
...nextConfig.experimental,
},
eslint: {
ignoreDuringBuilds: true,
...nextConfig.eslint,
},
typescript: {
// Motivated by https://github.com/vercel/next.js/issues/7687
ignoreBuildErrors: true,
...nextConfig.typescript,
},
};
}

module.exports = withDocsInfra;
7 changes: 4 additions & 3 deletions docs/pages/_document.js
View file
Open in desktop
Expand Up @@ -29,9 +29,10 @@ if (process.env.NODE_ENV === 'production') {
cleanCSS = new CleanCSS();
}

const PRODUCTION_DEPLOYEMENT = !process.env.PULL_REQUEST && process.env.NODE_ENV === 'production';
const PRODUCTION_GA =
process.env.DEPLOY_ENV === 'production' || process.env.DEPLOY_ENV === 'staging';

const GOOGLE_ANALYTICS_ID = PRODUCTION_DEPLOYEMENT ? 'UA-106598593-2' : 'UA-106598593-3';
const GOOGLE_ANALYTICS_ID = PRODUCTION_GA ? 'UA-106598593-2' : 'UA-106598593-3';

export default class MyDocument extends Document {
render() {
Expand Down Expand Up @@ -150,7 +151,7 @@ export default class MyDocument extends Document {
__html: `
window.ga=window.ga||function(){(ga.q=ga.q||[]).push(arguments)};ga.l=+new Date;
window.ga('create','${GOOGLE_ANALYTICS_ID}',{
sampleRate: ${PRODUCTION_DEPLOYEMENT ? 80 : 100},
sampleRate: ${PRODUCTION_GA ? 80 : 100},
});
`,
}}
Expand Down
8 changes: 6 additions & 2 deletions docs/src/components/footer/EmailSubscribe.tsx
View file
Open in desktop
Expand Up @@ -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 =
Copy link
Member

@Janpot Janpot Sep 12, 2022 •
edited

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,
  // ...
}

Copy link
Member Author

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.

Copy link
Member

@Janpot Janpot Sep 12, 2022 •
edited

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

Copy link
Member

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

process.env.DEPLOY_ENV === 'production' || process.env.DEPLOY_ENV === 'staging'
? 'https://f0433e60.sibforms.com/serve/MUIEAHEhgYhMvLAw0tycwk1BQaIB-q0akob3JdtDBmHLhSR-jLheJ2T44LFCz27alz9wq_Nkdz9EK7Y8hzM1vQND9kTFyKkkhTIbEzXaH5d-_S9Fw4PXS1zAK8efPY6nhCdoAop1SKTeZ_GAPW5S0xBFQRLUGYbvvRgE4Q2Ki_f1KjbiCqaRuzmj_I3SD1r0CoR4INmK3CLtF4kF'
: 'https://f0433e60.sibforms.com/serve/MUIEAE9LexIU5u5hYkoDJ-Mc379-irLHNIlGEgCm5njkAwg6OYFfNQTd25n4SO6vJom9WvQ89GJ0sYBzFYswLRewcOvD_dRtoFycXIObP8SMm-kNO1CdXKaWEZutrfqMKygHb1Je1QBGrMUnJg8J5qVeCwa7rSPBN0A1_6Ug3SiGjgIlbiCcMVA4KbhaYTiBvKkaejlCjgZcLHBT';

const Form = styled('form')({});

Expand All @@ -32,7 +36,7 @@ export default function EmailSubscribe({ sx }: { sx?: SxProps<Theme> }) {
event.preventDefault();
setForm((current) => ({ ...current, status: 'loading' }));
try {
await fetch(CONFIG.NEWSLETTER_SUBSCRIBE_URL, {
await fetch(NEWSLETTER_SUBSCRIBE_URL, {
method: 'POST',
headers: {
'Content-Type': 'application/x-www-form-urlencoded',
Expand Down
8 changes: 4 additions & 4 deletions docs/src/components/header/HeaderNavDropdown.tsx
View file
Open in desktop
Expand Up @@ -99,15 +99,15 @@ const DOCS = [
href: ROUTES.advancedComponents,
},
// @ts-ignore
...(process.env.STAGING === true
? [
...(process.env.DEPLOY_ENV === 'production'
? []
: [
{
name: 'MUI Toolpad',
description: 'Low-code admin builder.',
href: ROUTES.toolpadDocs,
},
]
: []),
]),
];

export default function HeaderNavDropdown() {
Expand Down
22 changes: 0 additions & 22 deletions docs/src/config.ts
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion docs/src/modules/components/DemoErrorBoundary.js
View file
Open in desktop
Expand Up @@ -55,7 +55,7 @@ export default class DemoErrorBoundary extends React.Component {
| Tech | Version |
|--------------|---------|
| MUI | v${process.env.LIB_VERSION} |
| netlify deploy | ${process.env.NETLIFY_DEPLOY_URL} |
| Netlify deploy | ${process.env.NETLIFY_DEPLOY_URL} |
| Browser | ${
typeof window !== 'undefined' && window.navigator
? window.navigator.userAgent
Expand Down
37 changes: 20 additions & 17 deletions docs/src/modules/components/DemoToolbar.js
View file
Open in desktop
Expand Up @@ -391,26 +391,30 @@ export default function DemoToolbar(props) {
});

const devMenuItems = [];
if (process.env.STAGING === true) {
if (process.env.DEPLOY_ENV === 'staging' || process.env.DEPLOY_ENV === 'pull-request') {
/* eslint-disable material-ui/no-hardcoded-labels -- staging only */
// eslint-disable-next-line react-hooks/rules-of-hooks -- process.env.STAGING never changes
// eslint-disable-next-line react-hooks/rules-of-hooks -- process.env never changes
const router = useRouter();

const defaultReviewID = process.env.GIT_REVIEW_ID ?? '20000';
if (process.env.PULL_REQUEST_ID) {
devMenuItems.push(
<MenuItem
key="link-deploy-preview"
data-ga-event-category="demo"
data-ga-event-label={demoOptions.demo}
data-ga-event-action="link-deploy-preview"
component="a"
href={`https://deploy-preview-${process.env.PULL_REQUEST_ID}--${process.env.NETLIFY_SITE_NAME}.netlify.app${router.route}/#${demoName}`}
target="_blank"
rel="noopener nofollow"
onClick={handleMoreClose}
>
demo on PR #{process.env.PULL_REQUEST_ID}
</MenuItem>,
);
}

devMenuItems.push(
<MenuItem
key="link-deploy-preview"
data-ga-event-category="demo"
data-ga-event-label={demoOptions.demo}
data-ga-event-action="link-deploy-preview"
component="a"
href={`https://deploy-preview-${defaultReviewID}--${process.env.NETLIFY_SITE_NAME}.netlify.app${router.route}/#${demoName}`}
target="_blank"
rel="noopener nofollow"
onClick={handleMoreClose}
>
demo on PR #{defaultReviewID}
</MenuItem>,
<MenuItem
key="link-next"
data-ga-event-category="demo"
Expand Down Expand Up @@ -451,7 +455,6 @@ export default function DemoToolbar(props) {
demo on&#160;<code>master</code>
</MenuItem>,
);

/* eslint-enable material-ui/no-hardcoded-labels */
}

Expand Down
4 changes: 2 additions & 2 deletions docs/src/modules/components/MuiProductSelector.tsx
View file
Open in desktop
Expand Up @@ -163,7 +163,7 @@ export default function MuiProductSelector() {
</Link>
</li>
{/* @ts-ignore */}
{process.env.STAGING === true ? (
{process.env.DEPLOY_ENV === 'production' ? null : (
<li role="none">
<Link
href={ROUTES.toolpadDocs}
Expand Down Expand Up @@ -193,7 +193,7 @@ export default function MuiProductSelector() {
/>
</Link>
</li>
) : null}
)}
</React.Fragment>
);
}
4 changes: 2 additions & 2 deletions docs/src/modules/sandbox/CodeSandbox.ts
View file
Open in desktop
Expand Up @@ -31,7 +31,7 @@ const createReactApp = (demo: {
};

const { dependencies, devDependencies } = SandboxDependencies(demo, {
commitRef: process.env.PULL_REQUEST ? process.env.COMMIT_REF : undefined,
commitRef: process.env.PULL_REQUEST_ID ? process.env.COMMIT_REF : undefined,
});

files['package.json'] = {
Expand Down Expand Up @@ -101,7 +101,7 @@ ReactDOM.createRoot(document.querySelector("#root")).render(
product: 'joy-ui',
},
{
commitRef: process.env.PULL_REQUEST ? process.env.COMMIT_REF : undefined,
commitRef: process.env.PULL_REQUEST_ID ? process.env.COMMIT_REF : undefined,
},
);

Expand Down
2 changes: 1 addition & 1 deletion docs/src/modules/sandbox/Dependencies.ts
View file
Open in desktop
Expand Up @@ -110,7 +110,7 @@ export default function SandboxDependencies(

// TODO: consider if this configuration could be injected in a "cleaner" way.
if ((window as any).muiDocConfig) {
const muiCommitRef = process.env.PULL_REQUEST ? process.env.COMMIT_REF : undefined;
const muiCommitRef = process.env.PULL_REQUEST_ID ? process.env.COMMIT_REF : undefined;
versions = (window as any).muiDocConfig.csbGetVersions(versions, { muiCommitRef });
}

Expand Down
2 changes: 1 addition & 1 deletion docs/src/modules/sandbox/StackBlitz.ts
View file
Open in desktop
Expand Up @@ -24,7 +24,7 @@ const createReactApp = (demo: {

const { dependencies } = SandboxDependencies(demo, {
// Waiting for https://github.com/stackblitz/core/issues/437
// commitRef: process.env.PULL_REQUEST ? process.env.COMMIT_REF : undefined,
// commitRef: process.env.PULL_REQUEST_ID ? process.env.COMMIT_REF : undefined,
});

return {
Expand Down