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

Any way to cut path as dependency? #16

Closed
rexxars opened this issue May 16, 2017 · 14 comments
Closed

Any way to cut path as dependency? #16

rexxars opened this issue May 16, 2017 · 14 comments
Labels
🙋 no/question This does not need any changes

Comments

@rexxars
Copy link

rexxars commented May 16, 2017

Not really sure whether this belongs here or in unified - anyway:

I'm using unified in a browser-facing module, which in turn pulls in vfile.
vfile uses path (the node module), which gets replaced with path-browserify, which in turn pulls in process.

This all ends up adding about 11.3 kb to the webpack bundle, of which my project will never really see any benefit. Is there any way to cut down on this?

All I'm really doing is unified().use(require('remark-parse')).parse(markdownString)

@wooorm
Copy link
Member

wooorm commented May 16, 2017

Good one!

From what I just saw, vfile just uses path.sep, path.join, path.dirname, path.basename, and path.extname, so we could be able to inline those. However, vfile also depends on replace-ext, which in turn depends on path as well.

Does webpack provide ways to exclude a file or module? Maybe that’s easier?

@rexxars
Copy link
Author

rexxars commented May 16, 2017

It does, and I probably would if this was in an app, but as I'm using it in a module, so don't think there is any way to tell anyone requiring my module to exclude it, unfortunately.

@wooorm
Copy link
Member

wooorm commented May 16, 2017

Would you be interested in creating a PR to inline these (including replace-ext)?

I kinda dislike that this will often be used in node though, and would add more code there, plus it would make file sizes bigger for projects that also include path, but if it’s not too big...

@rexxars
Copy link
Author

rexxars commented May 16, 2017

I was hoping there would be a better solution to the problem. I took a quick glance at some polyfill modules, and the amount of code required for even path.dirname looked pretty brutal (cross-platform and all that).

Don't have any immediate suggestion for how to approach this. Will give it some more thought.

@wooorm
Copy link
Member

wooorm commented May 16, 2017

Agreed! Let me know if you want to work on this or need help!

@wooorm
Copy link
Member

wooorm commented May 30, 2017

Hi @rexxars! I played around with this a bit today, and shaved some stuff off the GZipped size by cutting some unneeded stuff.

I also tried removing path and process, however:

  1. That means there’ll have to be a lot of code added and maintained!
  2. It’ll just duplicate code for people that also need process or path (pretty common in unified stuff)
  3. it doesn’t shave much off the GZipped size: 0.6kb.

The biggest one is the first: it’ll add so much maintenance, which I’d rather not take on! Unfortunately, that’s why I’m closing this. It’s an honourable and interesting goal, but I don’t like the trade-offs...

@wooorm wooorm closed this as completed May 30, 2017
@wooorm wooorm added the 🙋 no/question This does not need any changes label Aug 11, 2019
@kaisermann
Copy link

First of all, thanks for the work in this library and sorry for reviving this 😁 ! However, this library is a dependency of many browser related libs and projects and stating that Plus, they work in the browser. is misleading for the reasons mentioned here and in #38. I understand that having to keep maintain extra code is always a hard decision to make, but the current implementation don't work in browsers by default. Will try to tackle this one in a PR.

@wooorm
Copy link
Member

wooorm commented Jun 24, 2020

Hi there, thanks for the kind words! 👋

but the current implementation don't work in browsers by default

Essentially nothing on npm works in browsers by default. With dependencies, comes build tools. Those builds tools used to put path in there. Some don’t anymore. Could you provide more information about your build tools? We have answered many questions about build tools before

While we appreciate PRs, please don’t spend lots of time on something that may not be accepted, it may be a good idea to first discuss what you want to change.

@kaisermann
Copy link

kaisermann commented Jun 24, 2020

Hey @wooorm, thanks for the quick reply 🎉

We're using webpack, but with node mocks turned off in purpose. I completely understand the rationale behind nothing on npm works in browsers by default., but I think using modules built explicitly for node, instead of the browser, and expecting bundlers to just automagically add those dependencies can be a real issue. As stated in #38, webpack has decided to stop adding those node mocks/shims to our bundles by default. Apps built with rollup, if I'm not mistaken, would have the same problem, since it doesn't add any kind of shim for node libs.

And my bad, should've added a "will try to tackle this one in a PR if help is wanted and needed" 😅 ! One way to prevent having a lot of new code to maintain is to use the path shim that webpack itself uses: https://github.com/browserify/path-browserify/. The only code that would need to be created is the replace-ext method which also uses the path lib and is relatively small. Also, this change would add some extra weight to the library.

Even if these are not wanted, how would someone using, let's say, webpack 5 use any browser focused library that depends on this one?

@wooorm
Copy link
Member

wooorm commented Jun 24, 2020

I'll have to think about a proper solution. I think it's incredibly unfortunate that JavaScript is in such a horrible mess right now.

I don’t see any reason for webpack to break this stuff.

Re path-browserify: that was the default in the past, its what bundlephobia for this project already takes into account

@brodybits
Copy link

What about using path in peerDependencies or peerDependenciesMeta, which seems to be under 650 LOC on GitHub?

peerDependenciesMeta is newer and should not lead to warnings if the peer dependency is not used. Here are some resources with some more info & examples:

@wooorm
Copy link
Member

wooorm commented Jun 24, 2020

Hi Brody!

What about using path in…

path was last published 5 years ago, It’s not the same as path-browserify.


There are several ways to solve it. I think it’s not something library authors should deal with.

@ahuglajbclajep
Copy link

ahuglajbclajep commented Nov 24, 2020

Since the Node.js polyfills were removed in webpack@5, additional settings are required to run unified in a browser.
If you don't need to mdast and simply want to convert markdown to html, as in the first example, markdown-it is easier to use.

@ChristianMurphy
Copy link
Member

@ahuglajbclajep if you just want to go Markdown to HTML, without transforms, you may want to use https://github.com/micromark/micromark directly

wooorm added a commit that referenced this issue 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 <christian.murphy.42@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙋 no/question This does not need any changes
Development

No branches or pull requests

6 participants