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

feat(es2015): add es2015 implementation to support rollup #10

Merged
merged 6 commits into from
May 28, 2016
Merged

Conversation

benlesh
Copy link
Owner

@benlesh benlesh commented May 21, 2016

The build now comes from the es2015 files under the es/ directory.

closes #9

cc/ @gaearon for review.

The build now comes from the es2015 files under the `es/` directory.

closes #9
@@ -0,0 +1,17 @@
export default function symbolObservablePonyfill(root) {
let result;
let Symbol = root.Symbol;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

- let Symbol = root.Symbol;
+ const Symbol = root.Symbol;

@jamestalmage
Copy link
Collaborator

After looking at this, I am wondering if we haven't misidentified the problem in #9.

It doesn't seem right that Rollup would require you to rewrite existing CommonJS modules as ES2015 modules. This certainly isn't required when using babelify. I wonder if what's being reported in #9 isn't just a failure to properly configure Rollup (I've never used it, so I can't say for sure, but something seems off here).

@gaearon
Copy link
Contributor

gaearon commented May 22, 2016

It doesn't seem right that Rollup would require you to rewrite existing CommonJS modules as ES2015 modules

You can make it work by adding a special CommonJS plugin but vanilla Rollup will break. In Redux, we used to support vanilla Rollup specifically. Depending on this library breaks that support for us.

@jamestalmage
Copy link
Collaborator

@@ -12,7 +12,8 @@
"node": ">=0.10.0"
},
"scripts": {
"test": "mocha"
"build": "babel es --out-dir lib",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add build to the prepublish script as well:

"prepublish": "npm run build"

@gaearon
Copy link
Contributor

gaearon commented May 22, 2016

Yes, that one. If we ask people to use it, we might as well remove ES build from Redux completely because what's the point: they have to add it anyway. :-/

@benlesh benlesh merged commit 7a41de9 into master May 28, 2016
},
"files": [
"index.js",
"ponyfill.js",
"index.d.ts"
"index.d.ts",
"es/index.js",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you’d need to add "jsnext:main": "es/index.js" for Rollup to discover the ES build. The es folder name is just a convention I use.

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.

Add a Rollup-compatible ES modules build
4 participants