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

Handle browser code at build time (vs runtime) #679

Merged
merged 1 commit into from
Jul 25, 2021

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented Jul 19, 2021

Fixes https://github.com/pubkey/broadcast-channel/issues/636

I had to include #678 in this PR to be able to get anything done. If you'd prefer to merge that one first, I can rebase this one afterwards to drop those changes out of this PR

jsnext:main has been deprecated in favor of module so remove it
browser refers to the client-side entry point: https://docs.npmjs.com/cli/v7/configuring-npm/package-json#browser
main has to not use import and also provide the Node method, so I had to create a new build for that

Calling Babel a bunch of times from package.json feels a bit messy to me compared to having a single Rollup config that can output different builds. Rollup would also allow us to use @rollup/plugin-replace instead of sed for removing the Node method. However, this is a bigger change, so didn't want to do that as part of this PR

@benmccann benmccann force-pushed the browser-build branch 16 times, most recently from 8c5ac03 to 281bbc8 Compare July 20, 2021 15:24
@pubkey
Copy link
Owner

pubkey commented Jul 21, 2021

I merged #678
The code here looks good. But I noticed in the CI that this increased the build size dramatically.
For example the webpack build size goes from 4727 to 5479
Can you check if the node method is really correctly excluded from the builds?

@benmccann
Copy link
Contributor Author

Thanks for pointing that out. The reason webpack is bigger now is because I pointed it at the babelified version. I can verify that the node method is correctly excluded (I both see the code missing and verified that the webpack build fails if it were included)

Is the size increase an issue? If so, the best solution I think would be to not run the code through babel. Typically the end-developer should be responsible for this and not the library. E.g. for developers that need to only support newer browsers and are using Browserify today, they are getting a larger bundle than necessary because they are getting the Babelified version. Also, if a developer wants to support older browsers, they are typically going to have to setup Babel regardless (both for their own code and other libraries), so most of the time it's no extra work for them anyway

@pubkey pubkey merged commit f7b5588 into pubkey:master Jul 25, 2021
@pubkey
Copy link
Owner

pubkey commented Jul 25, 2021

Merged.
I am ok with pointing to the babeled version for now.
Thank you for the work, I will create a new release today.

@benmccann benmccann deleted the browser-build branch July 26, 2021 16:29
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

Successfully merging this pull request may close these issues.

ES version should not use require
2 participants