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

Update supported devEngines #21364

Merged
merged 4 commits into from
Apr 27, 2021
Merged

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Apr 27, 2021

Summary

Change devEngines to indicate the required node version to perform every development workflow.

With node@12.16.0 yarn build react-server-dom-webpack/node-loader throws with

$ node ./scripts/rollup/build.js react-server-dom-webpack/node-loader
/home/eps1lon/Development/forks/react/scripts/rollup/build.js:44
  throw err;
  ^

Error: Cannot find module 'react-server-dom-webpack/node-loader'
Require stack:
- /home/eps1lon/Development/forks/react/scripts/rollup/build.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:980:15)
    at Function.resolve (internal/modules/cjs/helpers.js:83:19)
    at createBundle (/home/eps1lon/Development/forks/react/scripts/rollup/build.js:556:13)
    at buildEverything (/home/eps1lon/Development/forks/react/scripts/rollup/build.js:776:11) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [ '/home/eps1lon/Development/forks/react/scripts/rollup/build.js' ]
}
error Command failed with exit code 1.

Probably because of the way exports field in packages/react-server-dom-webpack/package.json is constructed.

"exports": {
    ".": "./index.js",
    "./plugin": "./plugin.js",
    "./writer": {
      "react-server": {
        "node": "./writer.node.server.js",
        "browser": "./writer.browser.server.js"
      },
      "default": "./writer.js"
    },
    "./writer.node.server": "./writer.node.server.js",
    "./writer.browser.server": "./writer.browser.server.js",
    "./node-loader": "./esm/react-server-dom-webpack-node-loader.js",
    "./node-register": "./node-register.js",
    "./package.json": "./package.json"
  },

With node@12.17.0 the command passes.

Test Plan

  • CI green
  • yarn build react-server-dom-webpack/node-loader passes in node@12.17.0 and node@16.0.0

CI uses 12.19
@bvaughn
Copy link
Contributor

bvaughn commented Apr 27, 2021

The Code Sandbox documentation says:

  // Node.js version to use for building the PR.
  // Supported versions are '10' (10.23.0, default), '12' (12.20.0) and '14' (14.15.1).
  "node": "14"

And CI is failing with an error message that says:

AssertionError [ERR_ASSERTION]: Current node version is not supported for development, expected "10.23.0" to satisfy "^12.17.0 || 13.x || 14.x || 15.x || 16.x".

Which suggests to me that we should update the .codesandbox/ci.json config file to override the default of Node 10 with either 12 or 14, e.g.:

  "node": "14"

@sizebot
Copy link

sizebot commented Apr 27, 2021

Comparing: 9a25916...c683596

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 122.76 kB 122.76 kB = 39.42 kB 39.42 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 129.34 kB 129.34 kB = 41.51 kB 41.51 kB
facebook-www/ReactDOM-prod.classic.js = 406.92 kB 406.92 kB = 75.31 kB 75.31 kB
facebook-www/ReactDOM-prod.modern.js = 394.95 kB 394.95 kB = 73.41 kB 73.41 kB
facebook-www/ReactDOMForked-prod.classic.js = 406.92 kB 406.92 kB = 75.31 kB 75.32 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against c683596

@eps1lon eps1lon marked this pull request as ready for review April 27, 2021 15:36
@eps1lon
Copy link
Collaborator Author

eps1lon commented Apr 27, 2021

Which suggests to me that we should update the .codesandbox/ci.json config file to override the default of Node 10 with either 12 or 14, e.g.:

Good point. I went with the lowest possible version that's also being used in CI.

@bvaughn bvaughn merged commit 4edbcdc into facebook:master Apr 27, 2021
@bvaughn bvaughn deleted the chore/dev-engines branch April 27, 2021 16:03
@bvaughn
Copy link
Contributor

bvaughn commented Apr 27, 2021

Thanks~

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

Successfully merging this pull request may close these issues.

4 participants