createBroadcast is not a function

Question

createBroadcast is not a function

kentcdodds opened this issue 8 years ago · 38 comments

I really have no idea what's going on. But when I tried hooking this up with glamorous here I'm getting:

TypeError: createBroadcast is not a function
    at new ThemeProvider (dll.js:formatted:20246)
    at dll.js:formatted:9691
    at measureLifeCyclePerf (dll.js:formatted:9575)
    at ReactCompositeComponentWrapper._constructComponentWithoutOwner (dll.js:formatted:9690)
    at ReactCompositeComponentWrapper._constructComponent (dll.js:formatted:9683)
    at ReactCompositeComponentWrapper.mountComponent (dll.js:formatted:9638)
    at Object.mountComponent (dll.js:formatted:1790)
    at ReactCompositeComponentWrapper.performInitialMount (dll.js:formatted:9724)
    at ReactCompositeComponentWrapper.mountComponent (dll.js:formatted:9670)
    at Object.mountComponent (dll.js:formatted:1790)

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.

iamstarkov commented 8 years ago

okay

Answer 1 · 2017-06-09T15:33:37.000Z

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.

Answer 2 · 2017-06-09T15:46:34.000Z

@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.

Answer 3 · 2017-06-09T15:47:45.000Z

Then in some way @kentcdodds is not using that main module on the playground.

Answer 4 · 2017-06-09T15:53:44.000Z

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 :)

Answer 5 · 2017-06-09T15:55:40.000Z

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...

Answer 6 · 2017-06-09T15:56:09.000Z

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.

Answer 7 · 2017-06-09T15:58:47.000Z

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

Answer 8 · 2017-06-09T15:58:55.000Z

Time to plug rollup.

+1, it's the best for libraries 👍

glamorous has a good rollup config too.

Answer 9 · 2017-06-09T16:00:02.000Z

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.

Answer 10 · 2017-06-09T16:10:28.000Z

uhm...if theming does not bundle all the dependencies, how could it work without using webpack/browserify?

Answer 11 · 2017-06-09T16:34:11.000Z

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

Answer 12 · 2017-06-09T16:34:33.000Z

though theming@1.0 doesnt. i will investigate

Answer 13 · 2017-06-09T17:09:20.000Z

well, thats odd. it works in react-styleguidist's sandbox

Answer 14 · 2017-06-09T17:33:10.000Z

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 facebook/create-react-app#2485, they specifically stop Webpack from using that entrypoint because it seems to cause other problems as well.

Answer 15 · 2017-06-09T19:00:35.000Z

i guess, i need to expose both main and module entry points as well

Answer 16 · 2017-06-09T19:01:15.000Z

1.0.1 is ahead

Answer 17 · 2017-06-09T19:09:06.000Z

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

Answer 18 · 2017-06-09T19:10:43.000Z

Which seems weird to me

The reason is that webpack will "transpile" modules (ESM, CommonJS, AMD), but nothing else.

Answer 19 · 2017-06-09T19:13:06.000Z

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).

Answer 20 · 2017-06-09T19:20:19.000Z

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.

Answer 21 · 2017-06-09T19:37:12.000Z

This article's also relevant
http://2ality.com/2017/04/setting-up-multi-platform-packages.html

Answer 22 · 2017-06-09T23:11:54.000Z

I provided a PR to get the ball rolling.

Answer 23 · 2017-06-09T23:45:37.000Z

@gertt well, i do have PR as well =) #7

Answer 24 · 2017-06-09T23:50:32.000Z

Whoops 😀 Well, I think they're pretty much the same with different tools. ~~Think you forgot to add the new targets in package.json though.~~(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!

Answer 25 · 2017-06-09T23:51:40.000Z

@gertt missed entry points is a good catch, added it just now

Answer 26 · 2017-06-09T23:53:11.000Z

I prefer a rollup solution as the end result will be smaller and faster.

Answer 27 · 2017-06-10T00:02:47.000Z

@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?

Answer 28 · 2017-06-10T00:05:11.000Z

while we all are here, can you try and verify that temporary package @iamstarkov/theming-issue-5 fixes this issue?

Answer 29 · 2017-06-10T00:13:13.000Z

to clarify about rollup: im not against rollup, i just dont understand how bundling can help more than es modules

Answer 30 · 2017-06-10T05:43:14.000Z

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 :)

Answer 31 · 2017-06-10T06:01:01.000Z

Btw. webpack soon too.

Answer 32 · 2017-06-10T06:25:35.000Z

So excited!

Answer 33 · 2017-06-10T10:07:09.000Z

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 😀

Answer 34 · 2017-06-10T10:11:55.000Z

I think the upside of @iamstarkov's approach is that it leaves the choice up to the user:

If they use rollup (or anyhting with similar features), they can get the same benefits as if theming was built with rollup
If they use something without those features (webpack 1 is a good example I think?), they can still import only specific parts of the library if desired and not end up with the entire package

Answer 35 · 2017-06-11T01:29:35.000Z

well, 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?

Answer 36 · 2017-06-11T02:29:10.000Z

Sounds fine

Answer 37 · 2017-06-11T12:58:36.000Z

v1.0.1 is out there