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

"module" being browser-specific breaks bundling with Parcel for node #35

Closed
Andarist opened this issue Jan 29, 2020 · 10 comments · Fixed by #57
Closed

"module" being browser-specific breaks bundling with Parcel for node #35

Andarist opened this issue Jan 29, 2020 · 10 comments · Fixed by #57

Comments

@Andarist
Copy link

I know that this has somewhat been discussed in:

But the issue I'm facing is slightly different so I've decided to open a fresh one.

I understand why the current setup looks like it does and I can sympathize with you having to deal with that kind of issue as the current landscape of doing right by different tools is a mess (I also know this from experience 😉 ).

The problem is that assuming that "module" should contain only browser-specific code is a little bit of a stretch. Yes, it is primarily understood by bundlers which are used for building web apps most of the time but bundling for the node as a target is also a use case for them and historically no such limitation has been imposed on the files distributed as "module". It got introduced only to support a different authoring format, not the target platform.

I know you are supporting "browser" field and I would encourage you to put browser-specific code only there, otherwise people bundling this for node with Parcel or webpack will get broken builds.

@gr2m
Copy link
Owner

gr2m commented Jan 29, 2020

There is a consensus now over Conditional Exports. I'll implement that once Pika does, which they are working on. I expect parcel and webpack to adopt the standard, too.

@Andarist
Copy link
Author

I'm aware of this standard and I agree that most likely it will be supported soon-ish by other tools as well. We are not there yet and for a foreseeable future, it would still be a good thing to support older tooling. I expect conditional exports to take precedence over current fields (if both are defined), so it should be rather easy to set up the whole thing in a progressive enhancement manner.

@gr2m
Copy link
Owner

gr2m commented Jan 30, 2020

It's not as easy, I wish it was. Because the semantics of these fields are not defined, different build tools do different things with it. If I'll fix it for your use case, it gets broken for another. I've been doing this dance for years now.

There is usually some workaround for each use case / tool. They are inconvenient, but that's going to be the way until the tools all catch up with the standards.

@Andarist
Copy link
Author

It's not as easy, I wish it was. Because the semantics of these fields are not defined, different build tools do different things with it. If I'll fix it for your use case, it gets broken for another. I've been doing this dance for years now.

I'm also doing this dance for quite some time already and while - yes, some minor inconsistencies are there and there is no super formal semantics defined for those, but there are documents describing both "module" and "browser" fields to some extent.

If we take a look at the wiki page describing "module" field there is actually nothing said about it being "for browsers", it's purely about authoring module format. If we also take a look at webpack's docs (we might assume it's the leading bundler at the moment) about mainFields we might also notice that "module" is also used for node targets (again - not assuming it's only for browsers).

There is usually some workaround for each use case / tool. They are inconvenient, but that's going to be the way until the tools all catch up with the standards.

Truth to be told - there is no standard for "browser" fields or any others right now. Node's conditional exports only mention that those can be "created" (and most likely they will be), but no exact semantics are defined for this (AFAIK). It's really hard to introduce such without tight cooperation between bundlers as node after all don't want to decide about any of this. They have just solved their problem, partially having in mind a need for defining other targets and thus making the solution extendable.

If I'll fix it for your use case, it gets broken for another.

IMHO the appropriate solution would be to:

  • add a "browser" field that would cover aliasing for browser targets
  • change "module" to point to a node-compatible ESM file (or remove it altogether for now, as as we know "module" is not known to native node anyway and bundlers when targeting node can just use "main")

The only real downside I know of this is that Rollup requires an explicit opt-in for handling "browser" in their @rollup/plugin-node-resolve which is fine as "There is usually some workaround for each use case / tool". This is also a generic solution that is supposed to work with all packages when bundled with Rollup, while the current setup makes it broken by default when bundling for node target and requires a custom per-package solution.

Please assume my best intentions here, English is not my native language so I might sound harsh at times (it's harder for me to articulate myself in an appropriate polite manner).

@gr2m
Copy link
Owner

gr2m commented Feb 21, 2020

I'll add a try/catch around the navigator.userAgent, so at least it should no longer throw an error, via #41

Once conditional exports are supported by @pika/pack, I can provide custom exports for different environments, so the resulting user agent will be more helpful than <navigator unknown>, but at least this should unblock people for the time being

Any thoughts?

@Andarist
Copy link
Author

I still believe the issue stands on its own but definitely this try/catch is helpful 👍

@gr2m
Copy link
Owner

gr2m commented Feb 21, 2020

Agree, I'll leave the issue open until we have a proper solution via #38

@gr2m
Copy link
Owner

gr2m commented May 12, 2020

I've decided to greatly simplify this package to workaround the build problems y'all are experiencing. I was hoping for conditional exports to be adopted more quickly, but it does not look like it.

See #52

@github-actions
Copy link

🎉 This issue has been resolved in version 6.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link

github-actions bot commented Nov 4, 2023

🎉 This issue has been resolved in version 6.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants