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

Convert the code base to ES6 module #2558

Closed
oliviertassinari opened this issue Dec 16, 2015 · 14 comments · Fixed by #2614
Closed

Convert the code base to ES6 module #2558

oliviertassinari opened this issue Dec 16, 2015 · 14 comments · Fixed by #2614
Labels
core Infrastructure work going on behind the scenes

Comments

@oliviertassinari
Copy link
Member

This was started here #2333.
However, as pointed out by @apercu the index wasn't updated, same for the readme examples.
We can use https://github.com/reactjs/react-docgen/blob/master/src/main.js as inspiration.

I'm also wondering if adopting the ES6 module definition for the index would make webpack/browserify only imports the desired file with the following structure (as rollup.js).

import {DatePicker} from 'material-ui';`
@balthazar
Copy link
Contributor

I've done the work here, still have to be test it better before a clean PR. Don't hesitate if I've forgot something!

@oliviertassinari
Copy link
Member Author

@apercu That looks good! Could you submit a PR?
I guess that the following code would no longer work

import MUI from `material-ui`;
...
<MUI.TextField />

but those two would work

const MUI = require(`material-ui`);
...
<MUI.TextField />
const {TextField} = require(`material-ui`);
...
<TextField />

@alitaheri
Copy link
Member

and

import * as MUI from `material-ui`;

@balthazar
Copy link
Contributor

I also have to rework a bit on the lint to make it pass too

@newoga
Copy link
Contributor

newoga commented Dec 17, 2015

If we have the export extensions functionality enabled from babel, we might be able to put all these exports in a mui.js. Then in index.js do something like:

export * from './mui.js';
export {* as default} from './mui.js';

Haven't tested it but that should allow us to support:

import MUI from `material-ui`;

I know in the latest babel the export extensions are available in stage 1, but not sure about our version.

@mbrookes
Copy link
Member

mbrookes commented Jan 5, 2016

It looks like the reason my apps break at 0,14.1 might export related, as I'm still using require to import. What's odd is this change looks like it was introduced in 0.14.0, but that release still imports fine - and this wasn't listed as a breaking change in any case.

Was babel exporting both ways prior to 0.14.1, could the change to babel 6 have broken something? Or was this expected?

@alitaheri
Copy link
Member

@mbrookes for not, (before migrating to import) append ['default'] after your require call.

var AppBar = require('material-ui/lib/app-bar')['default'];

This should monkey patch it for now. @oliviertassinari This is an undocumented breaking change. What-to-do 😱 😱

@oliviertassinari
Copy link
Member Author

v0.14.0 is using babel 5. v0.14.1 is using babel 6.
It's depending on what version of babel you are using and, which plugin are enabled on your app side.
Babel added some breaking changes with the import (on their side).
So, that was kind of expected. I don't know the details here.

@alitaheri
Copy link
Member

This also happened with typescript on my side. luckily I always provide my own abstraction so that changes to libraries force me to only update one file. I guess we just have to stay sharp and tell anyone who encounters this issue what to do.

@oliviertassinari
Copy link
Member Author

Could this issue occures when people are not using babel correctly ?
With the following settings

@mbrookes
Copy link
Member

mbrookes commented Jan 5, 2016

@oliviertassinari - I'm using Meteor, with NPMs installed via browserify and babelify.

I won't pretend to know the details of those, as I've never had to look into it. It's entirely possible I'm using it incorrectly, although I followed the docs, and it has always just worked.

I also had a report from a user that formsy-material-ui imports were broken at MUI 0.14.0, but it was still working for me at that version, and I never got more details (their fix was to convert all the imports and exports to ES6 however). formsy-material-ui has no Meteor dependancy, and I'm using babel 5 command-line to transpile the lib itself. It currently uses require to import from material-ui.

@oliviertassinari
Copy link
Member Author

I won't pretend to know the details of those

Me neither.

@mbrookes
Copy link
Member

mbrookes commented Jan 5, 2016

Well, from a bit more research, this is definitely a breaking change introduced, rightly or wrongly by babel 6: https://phabricator.babeljs.io/T2212 "this change definitely shits in the commonjs interop punchbowl"

So babel 6 shouldn't have been introduced on a 0.x.x release, and ideally it should have a transition period, perhaps using something like https://www.npmjs.com/package/babel-plugin-add-module-exports.

Shall I open a new issue to continue discussion?

@oliviertassinari
Copy link
Member Author

@mbrookes Thanks for getting to the bottom of this 👍.
Using https://www.npmjs.com/package/babel-plugin-add-module-exports until v0.15 sounds like a good idea.

@zannager zannager added the core Infrastructure work going on behind the scenes label Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants