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

refactor: supports babel 7 #3

Merged
merged 9 commits into from
Sep 2, 2018
Merged

Conversation

christophehurpeau
Copy link
Contributor

Hello !

I tried this plugin and it didn't work with babel 7, because import is removed and binding lost.
The solution is to use Program.exit to remove it after.

Maybe you want to wait for the official babel 7 release before looking at this ?

Copy link
Owner

@avaly avaly left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the changes! I would like to wait for non-beta v7 babel before merging this.

But please revert the jest for now.

package.json Outdated
"classnames": "^2.2.5",
"eslint": "^4.1.1",
"eslint-config-prettier": "^2.3.0",
"glob": "^7.1.2",
"husky": "^0.14.2",
"jest": "^20.0.4",
"jest": "^22.4.3",
Copy link
Owner

Choose a reason for hiding this comment

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

Can you please revert this jest upgrade? I think the new version introduces some formatting changes to the snapshot file. I prefer this PR to just upgrade babel and any requirements for it.

I'll upgrade jest with a follow-up commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

jest had a direct dependency to babel-core, witch they fixed in jest 21: https://github.com/facebook/jest/blob/master/CHANGELOG.md#jest-2100

Change babel-core to peerDependency for compatibility with Babel 7

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can modify to 21 if you prefer

@@ -1,6 +1,4 @@
const styles = {
module.exports = {
Copy link
Owner

Choose a reason for hiding this comment

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

Any specific reason for changing this to commonjs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tests fails without the change:

● executes basic.js

    /Users/chris/Work/utils/babel-plugin-inline-classnames/test/styles.css.js:6
    export default styles;
    ^^^^^^

    SyntaxError: Unexpected token export

      at ScriptTransformer._transformAndBuildScript (node_modules/jest-runtime/build/script_transformer.js:316:17)
      at eval (eval at it (test/index.test.js:30:18), <anonymous>:5:38)
      at Object.it (test/index.test.js:30:18)

This ones also fails:

  • executes custom-import-name.js
  • executes object-expression.js
  • executes string-literal-mixed.js

@christophehurpeau christophehurpeau force-pushed the babel-7 branch 2 times, most recently from 3270ce6 to eb190fb Compare August 26, 2018 09:43
package.json Outdated
"classnames": "^2.2.5",
"eslint": "^5.4.0",
"eslint-config-prettier": "^3.0.1",
"glob": "^7.1.2",
"husky": "^0.14.3",
"jest": "^23.5.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

jest 23 is not compatible with babel 7:
jestjs/jest#4923

Copy link
Contributor Author

Choose a reason for hiding this comment

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

package.json Outdated
@@ -55,7 +57,7 @@
"validate-commit-msg": "^2.14.0"
},
"peerDependencies": {
"babel-core": "6.*"
"babel-core": "6.* || ^7.0.0-bridge.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you want to make a breaking change, I can update to "@babel/core": "7.*" ?

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, let's do that. I'll publish this as a major version bump anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done :)

BREAKING CHANGE: @babel/core is now a peer dependency and replace babel-core
package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
@christophehurpeau
Copy link
Contributor Author

christophehurpeau commented Sep 1, 2018 via email

@avaly avaly merged commit 5d2118a into avaly:master Sep 2, 2018
@christophehurpeau
Copy link
Contributor Author

Thanks !

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.

2 participants