Closed kentcdodds closed 7 years ago
Wondering how this ever worked, because @iamstarkov is not using es6 imports, he uses require, but brcast is exporting a default using es6 export default https://github.com/vesparny/brcast/blob/master/index.js#L1 which means when importing with require one would need to write require('brcast').default
@iamstarkov I think in any case its better to use import statements.
@kof brcast
's main module is this one https://unpkg.com/brcast@2.0.0/dist/brcast.cjs.js
That's the reason why it was working I guess.
Then in some way @kentcdodds is not using that main module on the playground.
I just looked at the theming dist
folder https://unpkg.com/theming@1.0.0/dist/.
Files are only transpiled with babel, but dependencies are not bundled. It's not a problem of module format, but a problem of missing modules :)
Hmmm... The weird thing to me is that the line that requires createBroadcast
doesn't ever run before the error's thrown. I think that's what's going on. Almost seems like a bug in webpack 🤔 I'm thinking that maybe using import
statements would avoid the bug though...
Time to plug rollup.
@iamstarkov you can take a look at this config I use on another project.
https://github.com/vesparny/rxhr/blob/master/rollup.config.js
I export cjs
es
and umd
bundles.
Not sure umd
makes sense in this case.
Maybe @kentcdodds knows more.
Hmmm... The weird thing to me is that the line that requires createBroadcast doesn't ever run before the error's thrown. I think that's what's going on. Almost seems like a bug in webpack 🤔 I'm thinking that maybe using import statements would avoid the bug though...
@kentcdodds you will still miss brcast
and all other dependencies because the are missing
Time to plug rollup.
+1, it's the best for libraries :+1:
glamorous has a good rollup config too.
you will still miss brcast and all other dependencies because the are missing
You'll notice that I actually explicitly add that to my dependencies in the sandbox so it should be working. Also, it's included in the package.json
of this package. I don't think that he has to bundle brcast
with the package. I'm not using the UMD build in the sandbox. I'm using the main
from package.json
. require
/import
works in that environment.
uhm...if theming
does not bundle all the dependencies, how could it work without using webpack/browserify?
theming@0.1.2 has been built and published the same way as 1.0.0 and it works in webpackbin https://www.webpackbin.com/bins/-Km8TglfWP84oYhDquT1
though theming@1.0 doesnt. i will investigate
well, thats odd. it works in react-styleguidist's sandbox
brcast
exposes a "module"
entry point in its package.json which exposes the ES6 code. Some bundlers (like Webpack 2) will use this entrypoint instead of "main"
. Which causes your require('brcast')
call to get the { default: createBroadcast }
result instead of the function itself.
IMO, the safest solution is for you to use import
. Because this will make sure it works with both module.exports
as well as ES6 exports, regardless of what entrypoint of brcast
your consumers' bundlers use.
See also https://github.com/facebookincubator/create-react-app/pull/2485, they specifically stop Webpack from using that entrypoint because it seems to cause other problems as well.
okay
i guess, i need to expose both main
and module
entry points as well
1.0.1 is ahead
It's complicated and I'm not very knowledgable on the subject, but form the webpack docs:
The key "main" refers to the standard from package.json, and "module" to a proposal to allow the JavaScript ecosystem upgrade to use ES2015 modules without breaking backwards compatibility.
"module" will point to a module that has ES2015 module syntax but otherwise only syntax features that browser/node supports.
So a "babel-ified" version of your package but with export
instead of module.exports
. (Which seems weird to me.)
Source: https://webpack.js.org/guides/author-libraries/#final-steps More info: https://github.com/rollup/rollup/wiki/pkg.module
Which seems weird to me
The reason is that webpack will "transpile" modules (ESM, CommonJS, AMD), but nothing else.
You can look at what we're doing over on glamorous with Rollup. We have a bit of a complicated setup with rollup, but it means that we can have several output variations and a great API for any module system (that doesn't require people to reference .default
when using the global or CJS).
Makes more sense to me now after reading more closely through the second link from my previous comment. Would probably be a good idea to take a similar approach to glamorous.
This article's also relevant http://2ality.com/2017/04/setting-up-multi-platform-packages.html
I provided a PR to get the ball rolling.
@Gertt well, i do have PR as well =) #7
Whoops 😀 Well, I think they're pretty much the same with different tools. Think you forgot to add the new targets in (fixed while I was typing) And did not include a UMD build, but I wasn't even sure if it was useful to add. So close mine if you like!package.json
though.
@Gertt missed entry points is a good catch, added it just now
I prefer a rollup solution as the end result will be smaller and faster.
@kentcdodds im not quite sold by rollup for libs.
can you explain a bit more why bundling libs will make apps smaller? i think if all libs are bundled on their own, then apps' bundles will be quite huge.
Isnt it enough that package provide esm version, and then it doesn't matter at which step tree shaking will happen. am i right?
while we all are here, can you try and verify that temporary package @iamstarkov/theming-issue-5
fixes this issue?
to clarify about rollup: im not against rollup, i just dont understand how bundling can help more than es modules
The biggest thing is that rollup is capable of doing what's called "scope hoisting." I don't have time to go into it now, but I found this blogpost which I think explains it :)
Btw. webpack soon too.
So excited!
If I understand correctly, the upsides of rollup are:
Tree shaking
Removes unused code from included dependencies. Which is only useful if you include the dependencies in the bundle. For example if @iamstarkov wants to provide a special browser bundle that can be loaded with a script tag instead of included in projects that use webpack/rollup/... themselves.
Scope hoisting
Moves all code into the same scope, making for a smaller bundle with less nesting etc which would also make it a bit faster.
Downside?
You can't do import createThemeProvider from "theming/dist/create-theme-provider";
anymore. However this should only be a problem for people who use theming
in an app whose build process does not support tree shaking. These people will end up with a larger bundle because they need to import everything even if they only want createThemeProvider
.
For me personally, most of the projects I work on would fall into the latter category. But that does not necessarily have to mean anything 😀
I think the upside of @iamstarkov's approach is that it leaves the choice up to the user:
theming
was built with rollupwell, both pull-requests successfully solve this issue. As @Gertt mentioned consumers' apps will benefit from both if they use rollup. To add to that, i'm more familiar with babel than with rollup. And anyway, switch between babel and rollup is quite easy, so if one time in the future we will need rollup build, thats gonna be easy.
That said, i would stick to my PR. what do you think?
Sounds fine
v1.0.1 is out there
I really have no idea what's going on. But when I tried hooking this up with glamorous here I'm getting:
The problem is in this file.
It's tricky debugging in codesandbox, but this is what the code looks like:
You'll see that I have a breakpoint on where we're requiring
createBroadcast
but that breakpoint never hits and it hits the_this.broadcast = ...
line first. Quite odd. Not sure what's going on here.