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

require process rather than using global #56

Closed
wants to merge 1 commit into from

Conversation

hanford
Copy link

@hanford hanford commented Aug 11, 2020

Hello maintainers!

First of all, thanks for working on vfile, I've been using this a lot (especially with remark) and it's been a lifesaver.

tl;dr, webpack versions 1-4 used to automatically polyfill node.js libraries and the next version, version 5, doesn't.

While migrating a larger project of mine, I ran into a scenario where vfile was breaking because it wasn't loading my process polyfill because vfile is using process as a global.

Here is the related webpack issue: webpack/webpack#11282

Hello maintainers!

First of all, thanks for working on vfile, I've been using this a lot (especially with remark) and it's been a lifesaver.

tl;dr, webpack versions 1-4 used to automatically polyfill node.js libraries and the next version, version 5 doesn't.

While migrating a larger project of mine, I ran into a scenario where vfile was breaking because it wasn't loading my `process` polyfill because `vfile` is using `process` as a global.

Here is the related webpack issue: webpack/webpack#11282
@hanford
Copy link
Author

hanford commented Aug 11, 2020

@wooorm instead of this approach, should I instead add a browser field to the package.json as well as include the built vfile.js in the files array?

@wooorm
Copy link
Member

wooorm commented Aug 21, 2020

Hi Jack!

a) I believe it is possible to configure webpack to include it? b) this change breaks CI

See also previous webpack issues in our org: https://github.com/search?q=org%3Avfile+webpack&type=Issues

@hanford
Copy link
Author

hanford commented Aug 26, 2020

Thanks for getting back to me @wooorm I thought I tried making that change, but I can try it again.

In the meantime, I'll close this PR.

@hanford hanford closed this Aug 26, 2020
@wooorm
Copy link
Member

wooorm commented Aug 26, 2020

Alright, please let me know how that goes!

@hanford hanford deleted the patch-1 branch August 27, 2020 19:30
@Gervwyk
Copy link

Gervwyk commented Nov 8, 2020

Any update on this? Similar issues with webpack 5 for us.

@wooorm
Copy link
Member

wooorm commented Nov 8, 2020

@Gervwyk this is a conversation about webpack 5. And it's a closed PR, there is not going to be an update.

wooorm added a commit that referenced this pull request Dec 6, 2020
Many new developers do not know how to configure webpack.
When they use a package that somewhere deep down uses `path` or `process` or
whatnot, through webpack 5, they mistakingly think this package does not work in
browsers.

This infuriating mistake by webpack leads to superfluous work for maintainers
and bigger bundles for users because polyfills add bloat.

See:
* <https://blog.sindresorhus.com/webpack-5-headache-b6ac24973bf1>
* <vercel/next.js#16022>

Note that this change shaves off 40% from the bundle size compared to including
`path` and `process`, however, it will double the bundlephobia size (because it
normally does not include path/process) and will hurt folks who depend on
another project that includes `path`.

Related to #16.
Related to #28.
Related to #38.
Related to #35.
Related to #56.
Related to #57.
Related to #58.
Related to remarkjs/react-markdown#492.
Related to remarkjs/react-markdown#514.

Reviewed-by: Christian Murphy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants