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

V5 add back support for node builtins #11764

Closed
wants to merge 8 commits into from
Closed

V5 add back support for node builtins #11764

wants to merge 8 commits into from

Conversation

raix
Copy link
Contributor

@raix raix commented Dec 15, 2021

Fixed #11756 by adding option to install nodeJS builtin fallbacks

  • Document the escape hatch and why not to use it
  • Improve the error message from Webpack 5 - make it more developer friendly pointing to the documentation
  • Maybe add an extra check of the package main fields (e.g. if the package has "browser" main field then advice to report an issue - it should not rely on nodejs built in modules)

@raix
Copy link
Contributor Author

raix commented Dec 15, 2021

Investigating if webpack config of mainfields is the root cause of the issue https://webpack.js.org/configuration/resolve/#resolvemainfields

@Rubenvet
Copy link

Do you mind explaining how to solve the issue step by step for a beginner. I keep getting the same issue, but I am still very new to all of this. Thank you!

@raix
Copy link
Contributor Author

raix commented Dec 17, 2021

Opened an issue in Webpack: webpack/webpack#15007

If a package like "handlebars" is imported, webpack fails to resolve import using the existing "browser" main field and fallsback to using the "main" field resolving the node js version of the package.

This ends up asking for node js builtin packages - if we add support for builtin packages we could risk adding backend code in the browser.

We might need to add documentation regarding this and some guidance for developers not experienced enough to realize the difference between npm packages
(developers have to read the documentation of the library they are using and make sure it's meant for the browser and not the backend... + Maintainers of polymorphic packages need to create browser and nodejs specific builds of their packages - we need to help fix this..)

@raix
Copy link
Contributor Author

raix commented Dec 17, 2021

@Rubenvet please use the issue tracker or stack overflow - this pull-request is for discussing a solution

@raix
Copy link
Contributor Author

raix commented Dec 17, 2021

I see the intent of the Webpack resolve.fallback option a tool for package maintainers - not consumers of packages

@raix
Copy link
Contributor Author

raix commented Dec 17, 2021

Suggestion for documentation / migration: #11756 (comment)

@raix
Copy link
Contributor Author

raix commented Dec 17, 2021

This PR set nodejs builin fallbacks to false in production and a debug fallback in development with error message and link to documentation if any code is using the module runtime:

image

@raix raix marked this pull request as ready for review December 17, 2021 17:56
@jodaka
Copy link

jodaka commented Dec 20, 2021

I tried upgrading few of my projects to 5.0.0 but they all stopped working cause process.env. is no longer available. And I couldn't find anything about it in documentation. Not sure if it's related to this PR

// Check app package.json for fallback dependency making sure we use project installed fallbacks
try {
// Use installed fallback
fallbacks[nodeModule] = require.resolve(fallbackModule);
Copy link
Contributor

@merceyz merceyz Dec 20, 2021

Choose a reason for hiding this comment

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

If you want to resolve it from the app you need to set the paths to point to it, otherwise you're resolving it from react-scripts itself which means you need to add them all as peer dependencies

| os | os-browserify/browse |
| path | path-browserif |
| punycode | punycode |
| process | process/browse |

Choose a reason for hiding this comment

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

Typo: should be process/browser (missing r at the end).

Copy link

Choose a reason for hiding this comment

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

At least half the names in the 2nd column seem to be missing their last letter:

  • browserif should be browserify ? (multiple variants)
  • stream-htt should be stream-http ?
  • readable-stream/duple should be readable-stream/duplex ?
    etc etc etc

Copy link
Contributor

Choose a reason for hiding this comment

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

We're looking at this at the moment, and just noticed this issue too. Thanks @no23reason and @PjotrB.

@krisgerhard
Copy link

Is fixing this issue still on track for the milestone 5.0.1?

liuderchi added a commit to liuderchi/EthereumCasts that referenced this pull request Jan 8, 2022
- workaround cra@5 builtin module issue, temporarily use react-scripts@4
- workaround: facebook/create-react-app#11764
liuderchi added a commit to liuderchi/EthereumCasts that referenced this pull request Jan 8, 2022
- workaround cra@5 builtin module issue, temporarily use react-scripts@4
- workaround: facebook/create-react-app#11764
- complete example code: https://docs.metamask.io/guide/ethereum-provider.html#using-the-provider

NodeJS builtin fallbacks enable you to import NodeJS builtin modules meant for Node and fallback to browser specific modules in your web application.

Per default Create React App set fallbacks to empty modules in production build and development fallbacks in development mode.

Choose a reason for hiding this comment

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

Suggested change
Per default Create React App set fallbacks to empty modules in production build and development fallbacks in development mode.
This restores the previous behavior in Webpack < 5, where Create React App used to include these builtin fallbacks.

I think this is how the sentence was intended, connecting to the previous sentence. I changed the inclusion to past tense.

@evandev01
Copy link

Webpack v5 has wrecked my React applications, and I pretty much can't code. This is a disaster. I have tried adding fallbacks, installing the million dependencies, downgrading react-scripts to 4.0.3. This is an EMERGENCY and needs to be FIXED NOW as literally 0 solutions work. Speaking of work my BUSINESS IS SUFFERING. Again this needs to be fixed IMMEDIATELY.

@benjsmi
Copy link

benjsmi commented May 17, 2022

There are now approximately 7-8 vulnerabilities in the NVD in npms that are depended upon by React 4.x -- would love to move to React 5.x, but can't because of the problem this PR fixes.

@rhenness
Copy link

rhenness commented May 17, 2022

not sure if it takes care of all use cases covered by this PR, but as an alternative, a relatively small craco config can be used to add the "node-polyfill-webpack-plugin" to the webpack config

craco.config.js

const NodePolyfillPlugin = require('node-polyfill-webpack-plugin')

module.exports = {
  webpack: {
    plugins: [
      new NodePolyfillPlugin({
        excludeAliases: ['console'],
      }),
    ],
  },
}

Careful, I think craco is only using react-scripts v4

@cmdcolin
Copy link

cmdcolin commented May 17, 2022

@ryan-henness-trimble that is a valid concern but for this basic usage, craco has worked fine for me :)

random add-on: I know also there are other ways to do it e.g. webpack recommends using "fallback" config field which might look like this, as an alternative to node-polyfill-plugin

// possibly alt craco.config.js method, untested though
 webpack: {
  resolve: {
    fallback: {
      buffer: require.resolve('buffer/'),
      url: require.resolve('url/'),
    },
  },
 }
// note: separately use `yarn add buffer` and `yarn add url` to allow the above to work

xref https://viglucci.io/how-to-polyfill-buffer-with-webpack-5

@pavlovp
Copy link

pavlovp commented May 24, 2022

In my opinion, polyfills are bad engineering design.
If the webapp imports libraries that use node-specific apis - should it compile with success?
Webpack allowed that for years - and this is why we got that mess.

@ljharb
Copy link

ljharb commented May 24, 2022

@pavlovp yes, it should; but it doesn’t matter if it should or not - it has for a decade, and it shouldn’t be changed now.

@jordanwilking
Copy link

jordanwilking commented May 24, 2022

@cmdcolin Thanks for the craco config!

For posterity, I had to add a plugins property for buffer (as well as npm install the individual polyfills):

webpack: {
    configure: {
        resolve: {
            /**
             * create-react-app 5 uses webpack 5, which no longer ships with node polyfills.
             * We use these polyfills throughout the project either directly or via a dependency.
             * If/when CRA allows us to add these back in (https://github.com/facebook/create-react-app/pull/11764),
             * that would be preferrable to using craco.
             * Craco's team is looking to give it up (https://github.com/gsoft-inc/craco/issues/415).
             * Possible alternatives: react-app-rewired or ejecting from CRA
             */
            fallback: {
                assert: require.resolve("assert"),
                buffer: require.resolve("buffer"),
                process: require.resolve("process/browser"),
                stream: require.resolve("stream-browserify"),
            },
        },
        plugins: [
            new webpack.ProvidePlugin({
                Buffer: ["buffer", "Buffer"],
            }),
        ],
    },
},

@filoscoder
Copy link

filoscoder commented Jun 21, 2022

For now, we solved this with react-app-rewired override

const Webpack = require("webpack");

module.exports = function override(config) {
  // Required polyfill packages
  config.resolve.fallback = {
    ...config.resolve.fallback,
    console: require.resolve("console-browserify"),
    crypto: require.resolve("crypto-browserify"),
    path: require.resolve("path-browserify"),
    process: require.resolve("process/browser"),
    stream: require.resolve("stream-browserify"),
    util: require.resolve("util"),
  };
  // Required global polyfill
  config.plugins = [
    ...config.plugins,
    new Webpack.ProvidePlugin({
      Buffer: ["buffer", "Buffer"],
      process: "process/browser",
    }),
  ];

  return config;
};

@tomtobac
Copy link

tomtobac commented Jul 7, 2022

Any update on this?

@meatnordrink
Copy link

For anyone searching for a solution to this in the meantime - here's a blog post (seems like from a company whose product was broken by this upgrade) outlining what seems to be the most common solution:

https://alchemy.com/blog/how-to-polyfill-node-core-modules-in-webpack-5

lutangar added a commit to datatlas-erasme/front that referenced this pull request Aug 17, 2022
This is an insane for such a small issue but there is no alternative for the time being.
See facebook/create-react-app#11756 and related PR facebook/create-react-app#11764.
@raix
Copy link
Contributor Author

raix commented Aug 17, 2022

Would be great if anyone are up for taking over this pr, currently out of time 🙏

@vjtyagi
Copy link

vjtyagi commented Sep 14, 2022

This is broken since last year, It should be the highest priority issue on the backlog. Does nobody on the core team has any time for fixing this issue? which is spoiling the whole developer experience of using CRA

@BearCooder
Copy link

The problem we see here is an assumption if something is using npm so it is technically using node and that is should polyfill everything you can do in node. PLEASE NO. Just because everyone does it it does not mean its correct - a terrible reality in npm land..

Thank god Webpack dropped that!

Overall the distinction between frontend and backend packages in the node ecosystem is bad (I would even say its horrific). But that does not mean in any world we should polyfill fs, crypto or other into webapps. This is a terrible idea, even considering the idea of polyfilling node builtins, because e.g. you DO NOT call crypto on your client.

Its a historical problem that finally gets eradicated (slow but steady) - CRA used to include polyfills for node js stuff that shouldnt have been running in the browser including crypto. If you imported node crypto modules directly in your component that should not ever have because nodes crypto module IS NOT a browser module but what they did for us was to polyfill those things. Finally React realized that this is probably not a good idea and the team made the right decision with v5 to not longer do this because doing that is just bad.

The vast majority of the things in node were built to run on the server because node is serverside javascript.
It got bastardized into a package manager for client apps but that doesnt mean we should just treat them the same.
Node builtin modules are not in any way built for the browser.

There is actually a new W3C working group who argue about base set of javascript commands and functionality should exist on all platforms ( be it node, deno etc).

So PLEASE DO not reintroduce back support for node builtins!

Despite the title this video is a great must watch for everyone that demands this comes back to CRA!! Just watch the first 20 minutes..

@ljharb
Copy link

ljharb commented Oct 5, 2022

@BearCooder while that's true that usage doesn't indicate correctness, this is actually the correct behavior. These are node modules, and a node module bundler should be providing these polyfills, or else it's broken.

Webpack is who made the decision, to be clear, not the CRA team - they either went along with it or failed to correct it when updating - and it's caused an inordinate amount of cost and pain for developers since. https://medium.com/sindre-sorhus/webpack-5-headache-b6ac24973bf1 covers it well.

WinterCG isn't relevant here except that it makes (according to your argument) is the opposite mistake - adding APIs designed exclusively for the browser to servers. If WinterCG makes sense, then polyfilling node core modules also makes sense - they're two sides of the same coin.

(Additionally, the npm ecosystem is the largest, and imo also the best, out of every language, but that's just an offtopic discussion)

@jodaka
Copy link

jodaka commented Oct 6, 2022

CRA docs says we can use process.env to get access to ENV variables. That was true for v4. Not anymore for v5.
Docs are misleading and should at least mention requirement to manually add polyfills in order to use ENV variables.

@jdempcy
Copy link

jdempcy commented Nov 18, 2022

[x-post from GitHub issue here]

For those just finding this, as of November 18 2022, here is the solution that worked for me:

Use react-app-rewired
I know some people are asking for solutions that do not use react-app-rewired, but unless you eject the app, this is the only way I've found to solve these issues. There may be another solution but this is what worked for me.

npm i react-app-rewired

Create config-overrides.js in the root with the following:

const webpack = require('webpack');

module.exports = function override(config, env) {
    config.resolve.fallback = {
        url: require.resolve('url'),
        fs: require.resolve('fs'),
        assert: require.resolve('assert'),
        crypto: require.resolve('crypto-browserify'),
        http: require.resolve('stream-http'),
        https: require.resolve('https-browserify'),
        os: require.resolve('os-browserify/browser'),
        buffer: require.resolve('buffer'),
        stream: require.resolve('stream-browserify'),
    };
    config.plugins.push(
        new webpack.ProvidePlugin({
            process: 'process/browser',
            Buffer: ['buffer', 'Buffer'],
        }),
    );

    return config;
}

Change your build scripts in package.json

  "scripts": {
    "start": "react-app-rewired start",
    "build": "react-app-rewired build",
    "test": "react-app-rewired test"
  },

Replace dependencies with similar packages

I had to do this twice, once for path and once for fs.

I ran:

npm i path@npm:path-browserify
npm i fs@npm:file-system

That fixed the build errors I was getting! I can now npm run build or npm start no problem :)

@BearCooder
Copy link

Guys better prepare to switch over to something else than Create React App.
reactjs/react.dev#5487

@Curtis-D
Copy link

Curtis-D commented Feb 5, 2023

Guys better prepare to switch over to something else than Create React App. reactjs/reactjs.org#5487

Vite seems like the obvious choice

@raix raix closed this by deleting the head repository Apr 3, 2023
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.

v5 used to include polyfills for node.js core modules by default