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

fix(gatsby): Move "react-dom-server" out of framework chunk #37508

Merged
merged 3 commits into from
Jan 24, 2023
Merged

fix(gatsby): Move "react-dom-server" out of framework chunk #37508

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1e1b552
initial
LekoArts Jan 20, 2023
cb9de5f
fix develop
LekoArts Jan 20, 2023
44b64d3
optional chaining
LekoArts Jan 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 23 additions & 13 deletions packages/gatsby/src/utils/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,16 @@ import { shouldGenerateEngines } from "./engines-helpers"
import { ROUTES_DIRECTORY } from "../constants"
import { BabelConfigItemsCacheInvalidatorPlugin } from "./babel-loader"
import { PartialHydrationPlugin } from "./webpack/plugins/partial-hydration"
import { satisfiesSemvers } from "./flags"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unused import


const FRAMEWORK_BUNDLES = [`react`, `react-dom`, `scheduler`, `prop-types`]

// This regex ignores nested copies of framework libraries so they're bundled with their issuer
const FRAMEWORK_BUNDLES_REGEX = new RegExp(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving regex creation to top. Better performance than creating the regex over and over again

`(?<!node_modules.*)[\\\\/]node_modules[\\\\/](${FRAMEWORK_BUNDLES.join(
`|`
)})[\\\\/]`
)

// Four stages or modes:
// 1) develop: for `gatsby develop` command, hot reload and CSS injection into page
// 2) develop-html: same as develop without react-hmre in the babel config for html renderer
Expand Down Expand Up @@ -582,12 +588,7 @@ module.exports = async (
framework: {
chunks: `all`,
name: `framework`,
// This regex ignores nested copies of framework libraries so they're bundled with their issuer.
test: new RegExp(
`(?<!node_modules.*)[\\\\/]node_modules[\\\\/](${FRAMEWORK_BUNDLES.join(
`|`
)})[\\\\/]`
),
test: FRAMEWORK_BUNDLES_REGEX,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In develop stage we don't need to change anything, react-dom/server is only relevant for build-javascript stage

priority: 40,
// Don't let webpack eliminate this chunk (prevents this chunk from becoming a part of the commons chunk)
enforce: true,
Expand Down Expand Up @@ -634,12 +635,21 @@ module.exports = async (
framework: {
chunks: `all`,
name: `framework`,
// This regex ignores nested copies of framework libraries so they're bundled with their issuer.
test: new RegExp(
`(?<!node_modules.*)[\\\\/]node_modules[\\\\/](${FRAMEWORK_BUNDLES.join(
`|`
)})[\\\\/]`
),
test: module => {
// Packages like gatsby-plugin-image might import from "react-dom/server". We don't want to include react-dom-server in the framework bundle.
// A rawRequest might look like these:
// - "react-dom/server"
// - "node_modules/react-dom/cjs/react-dom-server-legacy.browser.production.min.js"
// Use a "/" before "react-dom-server" so that we don't match packages that contain "react-dom-server" in their name
if (
module?.rawRequest === `react-dom/server` ||
module?.rawRequest?.includes(`/react-dom-server`)
) {
return false
}

return FRAMEWORK_BUNDLES_REGEX.test(module.nameForCondition())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We seem to use module.identifier() in other places but most often on the internet I've seen people use module.nameForCondition()

Copy link
Contributor

Choose a reason for hiding this comment

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

it doesn't really matter for this use case. nameForCondition returns the resource without any query parts.
https://github.com/webpack/webpack/blob/293e677b355da0d5bc4ddfc97d2afec114912711/lib/NormalModule.js#L375-L380

Copy link
Contributor

Choose a reason for hiding this comment

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

this change brakes gatsby-plugin-preact which assumes that test function is a regex

},
priority: 40,
// Don't let webpack eliminate this chunk (prevents this chunk from becoming a part of the commons chunk)
enforce: true,
Expand Down