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

Webpack: Reqire process.js for polyfills #55971

Merged
merged 1 commit into from
Sep 7, 2021
Merged

Conversation

noahtallen
Copy link
Member

@noahtallen noahtallen commented Sep 2, 2021

Changes proposed in this Pull Request

In #54793, a dependency is introduced on lib0. This causes our webpack build to fail with the following error:

ERROR in ../node_modules/lib0/environment.js 14:29-36
Module not found: Error: Can't resolve 'process/browser' in '/home/nt/source/wp-calypso/node_modules/lib0'
Did you mean 'browser.js'?
BREAKING CHANGE: The request 'process/browser' failed to resolve only because it was resolved as fully specified
(probably because the origin is strict EcmaScript Module, e. g. a module with javascript mimetype, a '*.mjs' file, or a '*.js' file where the package.json contains '"type": "module"').
The extension in the request is mandatory for it to be fully specified.
Add the extension to the request.
resolve 'process/browser' in '/home/nt/source/wp-calypso/node_modules/lib0'
  Parsed request is a module
  using description file: /home/nt/source/wp-calypso/node_modules/lib0/package.json (relative path: .)
    Field 'browser' doesn't contain a valid alias configuration
    resolve as module
      /home/nt/source/wp-calypso/node_modules/lib0/node_modules doesn't exist or is not a directory
      /home/nt/source/wp-calypso/node_modules/node_modules doesn't exist or is not a directory
      looking for modules in /home/nt/source/wp-calypso/node_modules
        existing directory /home/nt/source/wp-calypso/node_modules/process
          using description file: /home/nt/source/wp-calypso/node_modules/process/package.json (relative path: .)
            using description file: /home/nt/source/wp-calypso/node_modules/process/package.json (relative path: ./browser)
              Field 'browser' doesn't contain a valid alias configuration
              /home/nt/source/wp-calypso/node_modules/process/browser doesn't exist
      /home/nt/source/node_modules doesn't exist or is not a directory
      looking for modules in /home/nt/node_modules
        /home/nt/node_modules/process doesn't exist
      /home/node_modules doesn't exist or is not a directory
      /node_modules doesn't exist or is not a directory
 @ ../node_modules/lib0/buffer.js 8:0-39 77:24-37 80:26-39
 @ ../node_modules/yjs/dist/yjs.mjs 8:0-38 715:11-32
 @ ../node_modules/@automattic/isolated-block-editor/build-module/components/block-editor-contents/use-yjs/yjs-doc.js 1:0-27 13:18-25 38:24-45 101:32-55 116:10-25 121:10-25

Based on some investigation, this happens when webpack tries to alias the process variable in lib0/environment.js. Based on the error The extension in the request is mandatory for it to be fully specified, I changed process/browser polyfill to process/browser.js in our webpack config. This fixed the error in the React 17 PR.

Also, according to https://stackoverflow.com/a/65018686, it's helpful to add a dependency on process (though I'm not sure if this is needed), and we don't have to add the alias if we use ProvidePlugin.

Testing instructions

CI should pass. Also try yarn start locally.

@github-actions
Copy link

github-actions bot commented Sep 2, 2021

@noahtallen noahtallen requested a review from a team September 2, 2021 22:27
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Sep 2, 2021
@noahtallen noahtallen self-assigned this Sep 2, 2021
@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

Copy link
Contributor

@fullofcaffeine fullofcaffeine left a comment

Choose a reason for hiding this comment

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

CI tests pass, yarn start works fine starting Calypso without errors. After assets were compiled, I smoke tested Calypso and it looks fine 👍🏻

@scinos scinos merged commit 8fbac36 into trunk Sep 7, 2021
@scinos scinos deleted the try/fix-node-polyfills branch September 7, 2021 06:41
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Sep 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants