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: Add support for ECMAScript modules #69

Merged
merged 8 commits into from Mar 5, 2020

Conversation

cybercase
Copy link

Following the discussion #67 I've made some changes to add transpilation of es modules.
This should solve the issue with the new default behaviour of file-loader v.5 of enabling esModules by default.

Looking at how file-loader is implemented here, and agreeing with @carsonreinke, I would have opted for a simpler fix, like the one you received in #26 .
However, since you've expressed the preference of using babel to avoid risks, I've followed your preference.

Let me know if you're ok with this PR or if you need me to make further changes.

Starting from v. 5, file-loader switch to esModule by default
babel-core is required for transpilation, while the
babel-plugin-add-module-exports allows impor of transpiled modules
without using the .default property.
The reported Syntax error is slighly different when coming from
babel
@cybercase
Copy link
Author

cybercase commented Dec 30, 2019

After looking at the tests failing on node 6, I tried to find out what was wrong.
I debugged the issue to find out the cause, and unfortunately it's not related to this PR.

So, here's the problem.

In the .babelrc of this project it's loaded the babel-plugin-istanbul plugin when NODE_ENV is set to test.
Starting from version 5.1.0 of babel-plugin-istanbul, babel 7 is required for it to work properly, and starting from version 5.2.0 an error is thrown if babel 6 is used instead of babel 7; here is the PR where they've had the discussion and then added the version check.

Since in this project both package.json and package-lock.json refer to version 5.0.1 of that plugin, everything should work fine since we're bound to that version.

However, the npm version 3 that's coming with node 6 does not honor the package-lock.json file, and installs the latest version matching the semantic versioning of package.json, that is 5.2.0.

I can think of 2 possible fixes.

  • Add a shrinkwrap file to make sure that even npm v.3 installs the correct version of babel-plugin-istanbul
  • Change the semver string related to babel-plugin-istanbul to make sure it matches version only up to 5.0.X (maybe using ^5.0.1 < 5.1.0)

By the way, this should probably be done in a separate PR.

Let me know what you think

@carsonreinke
Copy link

...or drop Node 6. Is this still being used? Isn't it EOL?

@cybercase
Copy link
Author

@carsonreinke I also think that support for v6 could be dropped.

However, I'm not sure this PR will be reviewed soon. It's been almost a month since I've opened it, and haven't received any feedback. Do you know someone to mention who could help us in moving this forward?

@cybercase
Copy link
Author

@jannikkeye @jhnns
I've aligned this PR to master to resolve conflicts in package-lock.json.
Happy to see that node6 has been removed from travis ci and all checks are good now.

@carsonreinke
Copy link

@cybercase sorry for not getting back to you, for sure dropping Node 6.

My only thought about this too, this puts a dependency on Babel 6 for projects that use extract-loader. What if I wanted to use Babel 7 in my project and use extract-loader? I really don't see a way around that, unless some sort of peer dependency and Babel version check nonsense.

@cybercase
Copy link
Author

@carsonreinke as I said in the first comment of this PR, I agree with you. I liked the approach of #26 that doesn't depend upon any babel version at all. However, following the discussion in that same PR, I read that the maintainer has a different point of view... that's why I choose to use babel in this PR.

@jhnns
Copy link
Member

jhnns commented Feb 10, 2020

@cybercase Thanks ❤️ I will review this in this week.

@jhnns jhnns changed the title Chore/handle es modules feat: Add support for ECMAScript modules Mar 5, 2020
@jhnns jhnns merged commit 6034f23 into peerigon:master Mar 5, 2020
@jhnns
Copy link
Member

jhnns commented Mar 5, 2020

Awesome, thank you 👍

I decided to release it as major version bump because the usage of Babel might introduce unwanted/unknown side-effects in some project setups. I don't think that it's a problem but let's better be safe than sorry :)

github-actions bot pushed a commit that referenced this pull request Mar 5, 2020
# [5.0.0](v4.0.3...v5.0.0) (2020-03-05)

### Features

* Add support for ECMAScript modules ([#69](#69)) ([6034f23](6034f23))

### BREAKING CHANGES

* The extract-loader now uses Babel to transpile the bundle code for the current Node version. This is required in order to support the new file-loader which produces ECMAScript modules. This *should not* be a breaking change but since Babel is also known to be kind of brittle in regards of configuration we cannot guarantee that there might be unknown/unwanted side-effects in your project setup.
@github-actions
Copy link

github-actions bot commented Mar 5, 2020

🎉 This PR is included in version 5.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

3 participants