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

Drop serverside js in a frontend package/separate bundle process from the source process for better accessability #420

Closed
BearCooder opened this issue Dec 19, 2022 · 5 comments
Labels
enhancement New feature or request

Comments

@BearCooder
Copy link

Is your feature request related to a problem and use case? Please describe.

My goal is to build a dapp. As a framework I use React with Typescript.
To start I just created a new React project with npx create-react-app my-app --template typescript
Then I imported Beacon and did the Beacon example: https://docs.walletbeacon.io/getting-started/simple-example/
When I started the app on localhost I suddenly got a ton of polyfill errors.

One of many similar errors as example:

ERROR in ./node_modules/cipher-base/index.js 2:16-43
Module not found: Error: Can't resolve 'stream' in '/home/projects/beacon/node_modules/cipher-base'

BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
This is no longer the case. Verify if you need this module and configure a polyfill for it.

If you want to include a polyfill, you need to:
        - add a fallback 'resolve.fallback: { "stream": require.resolve("stream-browserify") }'
        - install 'stream-browserify'
If you don't want to include a polyfill, you can use an empty module like this:
        resolve.fallback: { "stream": false }

After quite some time of breaking walls I figured out that:

  1. Downgrading the react scripts package from v5 to v4.0.3 my app runs without any problems! We have the webpack configs in react-scripts and there were some changes, thats why it doesnt work with v5 currently.
    https://www.npmjs.com/package/react-scripts
    But we all can say that this is only a temporary solution. For now downgrading to v4 works but ultimately this option will somewhere in the future go away.

Beacon needs polyfill and thats an old method that works different in Webpack 5 and Webpack 5 is already around two years old. React is using the higher Webpack versions. While building a library you actually should not add polyfill and bundle the library. It is already on the client app. So if you have it and the client app has it, it can easily happen that the versions are different and this will cause conflicts (like in my example).

Considering that there are many different framework(Vue, angular, react..) all have itself different Webpack configs and if you supply that in the library prebundled - you cannot manage and update all different versions so all apps will work. Thats some intense overhead. You shouldnt bundle your package since it makes tree shaking more complicated for the consumer.

Now to my feature request to solve this:
So either you:

  1. Beacon does not add polyfill and bundle the library as the process is already done on the client app side. No conflicts can happen.

  2. Or you separate the bundle process from the source process (from your source library files). Because when people use react they can use it without this awful polyfill mistakes.
    Alternatively you could put the bundle process in an own package and distribute the files over npm.

It will make the distribution easier and the accessability will be better too.

Describe alternatives you've considered

  1. An alternative that actually IS NOT an alternative is to do to react eject and insert polyfill but when you do that there is no real sense to use react anyway. I just wanted to mention it as I saw it mentioned quite often, so again please dont do that. Its a no go because you lose all react benefits.

  2. I saw also CRACO as solution: Let me elaborate why this is the same bad but even more:

CRACO stands for create react app configuration ovverride. This is one of the obscurest packages ever made. What CRACO does is patch packages parts of create react app to let you do things create react app didnt build in. This is an override layer on top of create react app that assumes an exact version of CRA and then you have to rewrite all of the patches when anything changes in CRACO. Instead of going deeper to solve the problem we build another layer on top of it.
Also CRACO is no longer maintained because.. maintaining that is miserable..

Some more context:

The polyfills issue was introduced by Webpack and the libraries that use the packages Webpack doesn't polyfill anymore and React makes it difficult to configure.
Its good that Webpack did this step.

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..

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) - Create React App 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. Thats why the errors happen now.

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).

Sorry for making it so long! :)

@BearCooder BearCooder added the enhancement New feature or request label Dec 19, 2022
@BearCooder
Copy link
Author

All the other polyfill errors for documentation:
BTW I think the polyfill shizzle is also the reason why people struggle so much with react native.

ERROR in ./node_modules/readable-stream/lib/_stream_readable.js 43:13-37
Module not found: Error: Can't resolve 'buffer' in '/home/projects/beacon/node_modules/readable-stream/lib'

BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
This is no longer the case. Verify if you need this module and configure a polyfill for it.

If you want to include a polyfill, you need to:
        - add a fallback 'resolve.fallback: { "buffer": require.resolve("buffer/") }'
        - install 'buffer'
If you don't want to include a polyfill, you can use an empty module like this:
        resolve.fallback: { "buffer": false }

ERROR in ./node_modules/readable-stream/lib/_stream_writable.js 65:13-37
Module not found: Error: Can't resolve 'buffer' in '/home/projects/beacon/node_modules/readable-stream/lib'

BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
This is no longer the case. Verify if you need this module and configure a polyfill for it.

If you want to include a polyfill, you need to:
        - add a fallback 'resolve.fallback: { "buffer": require.resolve("buffer/") }'
        - install 'buffer'
If you don't want to include a polyfill, you can use an empty module like this:
        resolve.fallback: { "buffer": false }
ERROR in ./node_modules/readable-stream/lib/internal/streams/buffer_list.js 63:15-32
Module not found: Error: Can't resolve 'buffer' in '/home/projects/beacon/node_modules/readable-stream/lib/internal/streams'

BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
This is no longer the case. Verify if you need this module and configure a polyfill for it.

If you want to include a polyfill, you need to:
        - add a fallback 'resolve.fallback: { "buffer": require.resolve("buffer/") }'
        - install 'buffer'
If you don't want to include a polyfill, you can use an empty module like this:
        resolve.fallback: { "buffer": false }

ERROR in ./node_modules/ripemd160/index.js 3:13-37
Module not found: Error: Can't resolve 'buffer' in '/home/projects/beacon/node_modules/ripemd160'

BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
This is no longer the case. Verify if you need this module and configure a polyfill for it.

If you want to include a polyfill, you need to:
        - add a fallback 'resolve.fallback: { "buffer": require.resolve("buffer/") }'
        - install 'buffer'
If you don't want to include a polyfill, you can use an empty module like this:
        resolve.fallback: { "buffer": false }

ERROR in ./node_modules/safe-buffer/index.js 3:13-30
Module not found: Error: Can't resolve 'buffer' in '/home/projects/beacon/node_modules/safe-buffer'

BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default.
This is no longer the case. Verify if you need this module and configure a polyfill for it.

If you want to include a polyfill, you need to:
        - add a fallback 'resolve.fallback: { "buffer": require.resolve("buffer/") }'
        - install 'buffer'
If you don't want to include a polyfill, you can use an empty module like this:
        resolve.fallback: { "buffer": false }

@BearCooder
Copy link
Author

FYI, the very popular web3.js lib on eth resolved the polyfill/webpack issue in their lateste release.

I did not dive in how, so maybe its still more a kind of "patch" than right solution but what I mean to say is, its not only related to Tezos but people on Ethereum are tackling this issue and I hope the Beacon team will too.

@tbinetruy
Copy link

Hi! Any news on this? It's really problematic on our side because we can't upgrade create react app which brings a lot of bug fixes in their dependencies.

@BearCooder
Copy link
Author

I personally consider it bad practice to include code in the front-end that was ment for back-end usage.
From my experience I've seen developers of any experienced try adding back-end npm packages on the front-end.

Package maintainers should ideally build node or browser specific bundles.

This is my personal view I support the move in Webpack to not polyfill.
(Also see the release notes regarding webpack 5 - there's a note regarding this breaking change for further details)
Taquito has a similar problem, you can search in their issues section.

On another note, there is a discussion to drop CRA - reactjs/react.dev#5487

@godenzim
Copy link
Contributor

This come from dependencies of dependencies that is a bit out of our control at some point. It's a common issue in this space, we agreed it would be nice to address it, but we do not have the capacity to do it. A pull request addressing this would be welcomed.

@godenzim godenzim closed this as not planned Won't fix, can't repro, duplicate, stale Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants