-
Notifications
You must be signed in to change notification settings - Fork 240
chore: precompile with babel #257
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
Conversation
@@ -32,6 +32,8 @@ module.exports = { | |||
}, | |||
], | |||
'prettier/prettier': 'error', | |||
'node/no-unsupported-features/es-syntax': 'off', |
There was a problem hiding this comment.
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 } }]], |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
🎉 This PR is included in version 22.6.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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