Skip to content
This repository has been archived by the owner on Aug 4, 2021. It is now read-only.

babel-converted module with default comes out wrong #29

Closed
flying-sheep opened this issue Jan 12, 2016 · 25 comments
Closed

babel-converted module with default comes out wrong #29

flying-sheep opened this issue Jan 12, 2016 · 25 comments

Comments

@flying-sheep
Copy link

when trying to bundle redux, i get TypeError: _createStore.ActionTypes is undefined

the code in createStore.js is basically:

exports.__esModule = true
exports['default'] = createStore

var ActionTypes = { ... }
exports.ActionTypes = ActionTypes

function createStore(...) { ... }

which gets wrapped into createStore$1 followed by

var require$$3 = (createStore$1 && typeof createStore$1 === 'object' && 'default' in createStore$1 ? createStore$1['default'] : createStore$1);

var _createStore = require$$3;

and the failing code is var initialState = reducer(undefined, { type: _createStore.ActionTypes.INIT });

hmm, obviously _createStore gets assigned to the function createStore directly.

the failing line should convert to contain createStore$1.ActionTypes.INIT

isn’t there an effort to handle __esModule properly?

@Rich-Harris
Copy link
Contributor

Please provide the steps to reproduce! We can't help you otherwise.

@flying-sheep
Copy link
Author

here;

  1. babel-node test-build.js
  2. node test.bundle.js

@flying-sheep
Copy link
Author

there’s more. e.g. this file converts using babel --optional runtime into:

var _interopRequire = require('babel-runtime/helpers/interop-require')['default'];

which rollup can’t handle as well

@Rich-Harris
Copy link
Contributor

Have spent some time wrestling with this issue. It's tricky – the problem is that

var foo = require('foo');

is converted to

import foo from 'foo';

but it doesn't mean that in the context of a CommonJS module that was compiled from an ES6 module with Babel (foo refers to the module object which may or may not have a default property). I'm not sure how you'd solve that without introducing a whole lot of complexity (though I'm certainly open to ideas).

To be honest, I'm not sure it's the right problem to solve. It's insane to keep converting modules back and forth between different formats – you add cruft with each conversion. I think it makes a lot more sense to look at how we can use Redux directly as an ES6 module.

Here's how. We can't simply do this...

import { combineReducers } from 'redux/src/index.js';

...because Redux has a Babel 5 config that's incompatible with Babel 6 (assuming we're using Babel 6 to bundle our own source code), but we can use Rollup to create our own build of Redux. Bonus: you get a smaller (and tree-shakeable) version of Redux!

Long term, the best solution is to encourage library authors to distribute ES6 modules using jsnext:main.

Let me know if you have any questions about that setup.

@flying-sheep
Copy link
Author

hey, check out reduxjs/react-redux#256. this is something we can teach people to do if they want to support jsnext:main

Long term, the best solution is to encourage library authors to distribute ES6 modules using jsnext:main.

well, sadly that means that i basically have to

  1. try if a module works
  2. if it doesn’t because babel
    1. file a bug there for jsnext:main
    2. figure out a temporary workaround (e.g. add to babel includes, then import from the src folder if available)
    3. if there is no workaround, copy a vendored version into my tree

@dantman
Copy link

dantman commented Feb 5, 2016

You can add react-big-calendar as another package broken by this issue.

Uncaught TypeError: _formats.set is not a function```

Is the error. Which occurs because src/localizers/moment.js imports src/formats.js and presumably rollup can't set _formats (which has Babel's nice __esModule) with the correct object.


I don't think jsnext:main is a viable solution at all if this bug doesn't at least get a good enough workaround that almost everything will work with rollup with minimal effort.

There is certainly going to be a lot of pushback on requests to add jsnext:main to packages. Because we're not just asking package maintainers to add a rollup specific property to all their packages; we're actually asking them to do that and publish their package with the source, meaning a potential doubling of the size of their package due to the code duplication. All for the small group of rollup users.

Which presents a chicken and egg issue. As long as the rollup userbase is small many package maintainers aren't going to want to support jsnext:main (which to non-rollup users would feel like a degradation of the package) just for the few rollup users; as long as packages don't support jsnext:main and this bug doesn't have a workaround there will be a bunch of packages that just don't work in rollup; as long as rollup can't guarantee that the whole ecosystem of modules that work fine with browserify, webpack, etc... will work with rollup, many users aren't going to want to use rollup; which brings you back to the first issue, as long as people aren't moving to rollup, package maintainers have no reason to support it. Which could result in rollup dying instead of packages supporting it.

@flying-sheep
Copy link
Author

with the source

meh, that’s a common misconception caused by the second hard thing (naming things).

it’s not the source unless you only use module syntax and elsewise only node-compatible code. for packages requiring more babel transforms it’s actually a second, differently-compiled bundle that needs to be published

feel free to read all of reduxjs/react-redux#256 to evaluate the actual amount of pushback this can create.

just for the few rollup users

webpack 2 will also support es2015 module syntax; no idea about jsnext:main

@Victorystick
Copy link
Contributor

@dantman I want to clarify that jsnext:main isn't Rollup specific; in fact we didn't invent it. jsnext:main provides a migration path from CommonJS modules to ECMAScript modules. As @flying-sheep says, Webpack 2 could also make use of jsnext:main.

We recommend library authors to bundle their libraries (excluding external dependencies) into both CommonJS main and ECMAScript jsnext:main bundles to allow users to choose to use a legacy or modern module format. That's what this plugin does.

@flying-sheep
Copy link
Author

yeah, and people are already pushing back against the very idea of doing that.

@flying-sheep
Copy link
Author

besides i don’t like how node’s main works. there should be a canonical “root” to every node package, relative to which everything gets required, and everything outside should be inaccessible. in reality, the package.json main field means that there’s two effective entry points: the main script (relevant when doing require('package-name')) and the root directory of the package (relevant when doing require('package-name/wherever/the-fuck/things/really-lie'))

main should point to the only script in the package (which has no relative imports), or a directory having a index.js and containing everything that index.js imports relatively (and those relatively imported modules import relatively)

then require('package-name/sub-package') will always be logical and contain no shitty dist or src or lib or whateverthefuck, which you’ll have to find out by looking into the packages.

@Swatinem
Copy link

Swatinem commented Feb 5, 2016

Sorry if I may just intrude into this conversation, without having read all of it.

While I would love to be able to consume a library via jsnext:main, I would strongly encourage you to not advocate making it a bundle.

The reason is namespace re-exports. Consider this example.

The library re-exports a namespace import, and as long as those are still in separate files, rollup can deal with it just fine and eliminate the things from the namespace export that I don’t use. When you pre-bundle that library for jsnext:main, that ability is lost, since rollup will turn that namespace re-export into an object. See this example.

And just fyi, my specific usecase is this:
I would like to essentially fork https://github.com/toji/gl-matrix and convert it to es2015 modules. The goal is to be able to just import {vec3} from 'gl-matrix'; and use certain functions and have everything else I do not use be eliminated. But that can’t happen if I pre-bundle, because that would lose the information that vec3 is just a namespace import internally. I hope you understand what I mean.

@Swatinem
Copy link

Swatinem commented Feb 5, 2016

Just playing around with this, I’m actually surprised that rollup can deal with deeply nested namespace re-exports, like here. That’s 4 files deep. I stopped trying more, maybe it could deal with a potentially arbitrary depth.

@Victorystick
Copy link
Contributor

@Swatinem I understand your concern with namespaces. I believe that the ease-of-use that comes with exporting bundled & transpiled code via jsnext:main is currently worth the loss in efficiency, but I'd like to see a solution developed that doesn't have this issue.

I’m actually surprised that rollup can deal with deeply nested namespace [...] maybe it could deal with a potentially arbitrary depth.

Indeed it can. 😃

@flying-sheep
Copy link
Author

@Rich-Harris

Here's how. We can't simply do this...

which involves an ad-hoc-update and looking up and reusing redux’ .babelrc

if we want to compete with other tools, this using redux properly needs to be one line of config total, not spread over a dozen lines in multiple files.


another problem is libs relying on babel’s liberal interpretation of how commonjs modules should work when imported via ES6, i.e. try importing code containing this line and it’ll fail since React is interpreted to only export default React without export { Component, ... }

@dtothefp
Copy link

dtothefp commented Mar 9, 2016

@Victorystick you can add Facebook's parse to this list. Unfortunately, their NPM module does not include the pre-compiled es6 code so we can't just use some sort of include/exclude combo to fix the issue. Seems crappy that there are so many blockers using Rollup. I've referenced this issue in two different places before I realized this was happening because of parse's pre-compiled babel code.

Any solutions or are we just out of luck if code is pre-compiled?

rollup/rollup-plugin-babel#13 (comment)
rollup/rollup-plugin-node-resolve#10 (comment)

@balupton
Copy link

balupton commented Apr 22, 2016

Seems related to heroku/react-refetch#114 react-refetch publishes only the compiled edition that is compiled with webpack and babel

Reading the comments above, could my work with https://github.com/bevry/editions help this?

@dtothefp
Copy link

@balupton unfortunately the only solution I found was to marke Parse as external and then load it via CDN or other method. Unfortunately in the end there were too many gotchas like this with roll up when applying to a medium/large scale app and I reverted back to webpack. Also you notice none of the maintainers seems to respond to github issues reliably so it makes it hard to figure out these small blocking issues

@download13
Copy link

This is a blocking bug for me using rollup. It seems much more intuitive and easy to use than webpack, but I really need redux.

@raphaelokon
Copy link

Same here. I was already using rollup for other projects. But today I wanted to start a small react/redux project and was wondering rollup is not (yet) supporting redux.

@Daniel15
Copy link

Why was this closed? Was something done to resolve the issues people were encountering with using Redux with Rollup?

@AlexGalays
Copy link

@Daniel15 redux has a jsnext:main field in its package.json since July 26. It should work with rollup now ?
@TrySound, that was a bit of a blunt close ^^

@TrySound
Copy link
Member

TrySound commented Sep 7, 2016

@AlexGalays Redux has module field enabled here by default last three days. Redux is compilled quite well in my projects.

@ngryman
Copy link

ngryman commented Jun 7, 2017

@TrySound I'm curious how? I still have the issue several months later. Did I miss something obvious or is it still not fixed?

With jsnext or module:

Error: 'connect' is not exported by node_modules/redux/es/index.js
https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module
src/hocs/auth.js (3:9)
1: import _Object$assign from 'babel-runtime/core-js/object/assign';
2: import React from 'react';
3: import { connect } from 'redux';
4:              ^
5: import createHOC from '../create-hoc';

Without jsnext or module:

Error: 'connect' is not exported by node_modules/redux/lib/index.js
https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module
src/hocs/auth.js (3:9)
1: import _Object$assign from 'babel-runtime/core-js/object/assign';
2: import React from 'react';
3: import { connect } from 'redux';
4:              ^
5: import createHOC from '../create-hoc';

EDIT: a public trace of sleep deprivation.

@TrySound
Copy link
Member

TrySound commented Jun 7, 2017

@rgrymam I guess it's because redux doesn't have connect util. It's just wrong package. You should use react-redux for that.

@ngryman
Copy link

ngryman commented Jun 7, 2017

... and I miss something obvious 😅, I need some sleep. Thanks and sorry.

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

No branches or pull requests