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

chore: precompile with babel #257

Merged
merged 1 commit into from May 12, 2019
Merged

chore: precompile with babel #257

merged 1 commit into from May 12, 2019

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented May 10, 2019

Prerequisite for #256. This has no TS yet, but sets up babel for jest and publishing.

This also allows us to use newer syntax while maintaining node 6 compat

@SimenB SimenB requested a review from macklinu May 10, 2019 18:04
@@ -32,6 +32,8 @@ module.exports = {
},
],
'prettier/prettier': 'error',
'node/no-unsupported-features/es-syntax': 'off',
Copy link
Member Author

Choose a reason for hiding this comment

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

babel deals with syntax

'use strict';

module.exports = {
presets: [['@babel/preset-env', { targets: { node: 6 } }]],
Copy link
Member

Choose a reason for hiding this comment

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

Why not Node 8 and next release major?

Copy link
Member Author

Choose a reason for hiding this comment

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

No need to make a breaking change, exactly 0 benefit

Copy link
Member Author

Choose a reason for hiding this comment

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

whenever we wanna make a new major we can drop 6 though, no reason to keep it at that point

"engines": {
"node": ">=6"
},
"peerDependencies": {
"eslint": ">=5"
},
"scripts": {
"postinstall": "yarn build",
Copy link
Member

Choose a reason for hiding this comment

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

Is auto-build after install helpful in this case?
Unlike in the Jest repo, you'll be able to run yarn jest just fine.
In development, yarn build --watch will initially build everything anyway (afaik that's what Babel CLI watch does)

Copy link
Member Author

Choose a reason for hiding this comment

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

Lint will fail since we run with our own rules. And jest runs lint, so it'd fail

Copy link
Member

Choose a reason for hiding this comment

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

Okay, didn't consider that 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

@SimenB postInstall runs when this package is being npm/yarn installed.

We can't assume that the user has yarn or any of the devDependencies.

I think we should revert it for now.

Copy link
Contributor

@ranyitz ranyitz May 22, 2019

Choose a reason for hiding this comment

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

Maybe the intention was pretest? I've created #265

@SimenB SimenB merged commit 48e3a59 into master May 12, 2019
@SimenB SimenB deleted the babel branch May 12, 2019 16:17
@SimenB
Copy link
Member Author

SimenB commented May 22, 2019

🎉 This PR is included in version 22.6.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