-
Notifications
You must be signed in to change notification settings - Fork 62
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
build: ditch transpilation #218
Conversation
1fee01b
to
3c613ac
Compare
3c613ac
to
443ea5f
Compare
8c0f911
to
7844196
Compare
7844196
to
6233669
Compare
Codecov Report
@@ Coverage Diff @@
## master #218 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 4 4
Lines 92 96 +4
Branches 4 4
=====================================
+ Hits 92 96 +4
Continue to review full report at Codecov.
|
6233669
to
c911c8f
Compare
Builds on Windows with Node 4 fails with a dependency missing error: - https://tinyurl.com/ycf38c4h Not really sure why this is needed, but it seems to be related to the usage of @kentcdodds' eslint config (https://git.io/vF5Fe). Here's some more details: https://git.io/vF5d5.
c911c8f
to
b6896b8
Compare
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.
LGTM so far, just two things:
-
Should we move the code from
lib/install.js
intobin
(git mv
and slightly alter the code to invoke theinstallHooks
function immediately)? Might be only "cosmetical". -
Does the
build
prefix trigger a release (I'm genuinely not sure)? I think I would have chosen sth. likechore(build)
which does not iirc.
👍 for moving install into Running
Which apparently is correct (patch release: |
I would interpret it as a breaking change to not support Node prior to v4 anymore (regardless of the reason behind), so I'm definitely against it being only a patch release. |
Oh yeah, definitely! Quit supporting node 4 is definitely a breaking change. I did think these changes would break compatibility, however everything is still fine with node 4 (and above): Apparently we were only using babel for destructing a few requires, which I addressed like the following: -const {resolve, basename} = require('path')
+const resolve = require('path').resolve
+const basename = require('path').basename |
So if it is a breaking change, it should be a major release then, shouldn't it? (Sorry for being so nitpicking 🙈) |
No need to apologize! We're just trying to get on the same page… 😉 I do agree that if it is a breaking change, it should be a major release (as a general rule). Although that is not the case with the changes on this PR in particular. We didn't break compatibility with node 4. Initially, I though we would have to stop supporting node 4, but it turns out it was very straightforward to remove babel from the game and keep the code compatible with node 4. So, unless I'm not seeing something, we're good to tag these changes as patch… (?) |
Let‘s do it 👍 Nevertheless, update the Readme / unmark as deprecated prior to releasing? Or do we wait until the first feature release? |
Cool. Does that make sense? I feel like there's something iffy… (interpreting people's reaction from textual comms is hard) 😄 I guess we can do it (update readme and remove deprecation notice) as soon as |
It’s all good. I‘m not a native english speaker, so things might sound different as intended. I only worried about the package’s readme over at npmjs which will be the version at the time of publishing the release (that is, it might lead to reactions like „why releasing an update when it’s deprecated?“). So, from my end, let‘s go 🙂 |
I get you now… Over at npm it'll still read as deprecated. That's indeed a valid concern! How about this, then: I'll remove the deprecation note from the readme, add it as part of this PR and then we merge and call it ps: I'm not native english speaker either 😃 |
Awesome! |
Builds on Windows with Node 4 fails with a dependency missing error: - https://tinyurl.com/ycf38c4h Not really sure why this is needed, but it seems to be related to the usage of @kentcdodds' eslint config (https://git.io/vF5Fe). Here's some more details: https://git.io/vF5d5.
Addresses #217.