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

Don't transform modules with import/export #96

Merged
merged 2 commits into from Sep 16, 2016

Conversation

Projects
None yet
5 participants
@Rich-Harris
Copy link
Contributor

commented Sep 2, 2016

Even if they contain CJS features like require, global etc. Ref #88 (comment)

@talmobi

This comment has been minimized.

Copy link

commented Sep 8, 2016

Had this issue with the recently released newest version of redux@3.6.0 (https://github.com/reactjs/redux/releases) which updated its symbol-observable dependency to version 1.0.2 (https://github.com/blesh/symbol-observable/blob/master/CHANGELOG.md) with es6 support.

A temporary fix is to simply ignore symbol-observable specifically as an exclude field in the commonjs plugin.

I just want to say that I tested this PR and it worked out of the box with the newest version of redux@3.6.0 and its updated symbol-observable dependency (and as a result fixed my build and dev npm scripts)

People using redux with the hat character ^ in their dependencies (like I did) will at some points auto update to 3.6.0 and come across this issue that this PR seems to fix.

The only there difference is that using this PR rollup-plugin-version logs out a message when running rollup/build script (not an error)

The `this` keyword is equivalent to `undefined` at the top level of an ES module, and has been rewritten

Other than that everything seems to be working the same.

references: benlesh/symbol-observable#21

@TrySound

This comment has been minimized.

Copy link
Member

commented Sep 8, 2016

@talmobi You don't need this plugin anymore to build redux. Just set rollup option context: 'window'.

@talmobi

This comment has been minimized.

Copy link

commented Sep 8, 2016

Ahh, very good, thanks -- still need the plugin for other libraries though, and unless you exclude symbol-obvservable you get the build error without this PR.

@danbucholtz

This comment has been minimized.

Copy link

commented Sep 8, 2016

I have verified that this fixes #100

@benlesh

This comment has been minimized.

Copy link

commented Sep 16, 2016

Thanks for your work on this @Rich-Harris

@Rich-Harris Rich-Harris merged commit 4fee437 into master Sep 16, 2016

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@Rich-Harris Rich-Harris deleted the unambiguous branch Sep 17, 2016

@Rich-Harris

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2016

Sorry for the delay everyone! Just released this as 5.0.0 since it's potentially a breaking change, along with another PR (#106) which should bring bundle sizes down a bit.

@timdorr timdorr referenced this pull request May 9, 2017

Merged

Remove CommonJS call #252

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.