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

Bundling with webpack will fail with default settings #1683

Closed
Domiii opened this issue Jan 23, 2020 · 6 comments
Closed

Bundling with webpack will fail with default settings #1683

Domiii opened this issue Jan 23, 2020 · 6 comments

Comments

@Domiii
Copy link

Domiii commented Jan 23, 2020

Due to #1538, webpack builds will fail by default (workaround provided).

Considering how popular webpack is, would be great if there can be a more versatile solution. As this guy points out, it is easily fixed by just not using the package.json browser field anymore. Any thoughts?

(PS: I just spent hours debugging this engine.io bug not allowing me to connect, because they accidentally silenced the error. - Not saying that this is your fault, just referencing.)

@lpinca
Copy link
Member

lpinca commented Jan 23, 2020

I'm 👎 on removing the browser field because if we do that people will start opening issues again about not being able to bundle for the browser like it was done countless of times in the past despite it being documented https://github.com/websockets/ws#ws-a-nodejs-websocket-library.

@Domiii
Copy link
Author

Domiii commented Jan 23, 2020

@lpinca You can just add an if in index.js instead - or have browser.js straight-up raise an error, rather than letting them require/bundle it and waiting until way later to let them know?

If the consensus is to not change the code, I would vote in favor of at least amending the README with a Common Problems section, specifically getting into the broken webpack default behavior of targeting the package.json's browser field even in a node or umd build? (Probably need to knock on socket.io's door as well, as that is probably one of your bigger consumers)

(PS: sorry for accidentally clicking the Close Issue button - it did not even prompt for confirmation :/)

@Domiii Domiii closed this as completed Jan 23, 2020
@Domiii Domiii reopened this Jan 23, 2020
@lpinca
Copy link
Member

lpinca commented Jan 25, 2020

To be honest I think everything works as expected here:

  • Bundlers default target is the browser. It has always been like this. I think it makes sense, and I don't expect it to be changed any time soon.
  • When the bundle is built for the browser an error is thrown. We could throw the error when the module is loaded instead of throwing it when calling the constructor but I think it does not make a big difference as the bundle would be generated anyway. The error will only be thrown at run time.
  • The bundler should be configured to produce a bundle for a specific environment. webpack has the target option for this.

@Domiii
Copy link
Author

Domiii commented Jan 25, 2020

@lpinca Ok I agree. Might want to mention it in the README though :)

(PS: I deleted my previous comment which was too much of a rant. Also thanks for pointing out the target option. Turns out that it itself is not properly documented, however light is shone on it in the resolve.mainFields documentation, which ironically I looked at before; must have been blind not to see the explanation there.)

@lpinca
Copy link
Member

lpinca commented Jan 25, 2020

I'm not sure what to add to the README. Documenting how to bundle an app it's out of the scope of this project and redirecting the user to the webpack documentation does not make much sense as the user may use a different bundler.

@lpinca
Copy link
Member

lpinca commented Jan 26, 2020

I'm closing this. Feel free to open a PR to add/change what you see fit.

@lpinca lpinca closed this as completed Jan 26, 2020
@Domiii Domiii changed the title Bundling with webpack will fail by default when not targeting browser Bundling with webpack will fail with default settings Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants