refactor: drop support for versions below Node.js 6 #125
Conversation
|
Thanks @Siilwyn for taking a hit at this! A couple of notes:
|
|
@Siilwyn This looks good! I think we can remove support for node 6 as well. Its maintenance period ended in April. Also, can you add a note on the readme saying to install cross-spawn@6 to support older node versions? |
Codecov Report
@@ Coverage Diff @@
## master #125 +/- ##
==========================================
- Coverage 98.88% 98.77% -0.12%
==========================================
Files 7 7
Lines 180 163 -17
Branches 39 33 -6
==========================================
- Hits 178 161 -17
Misses 2 2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #125 +/- ##
========================================
+ Coverage 98.88% 100% +1.11%
========================================
Files 7 7
Lines 180 160 -20
Branches 39 32 -7
========================================
- Hits 178 160 -18
+ Misses 2 0 -2
Continue to review full report at Codecov.
|
|
👍 I've changed to code following your feedback, please take another look! |
|
Looks good! @ehmicky wanna give any final feedback? |
|
Looks good to me, except one thing: shouldn't Travis run both for |
|
You are right, we need to define 8 explicitly in travis.yml. |
|
Also, appveyor.yml needs to be updated accordingly. |
|
Thinking about this I would propose to be explicit for all tested versions, this way the CI doesn't suddenly break and it is easy to see which versions are tested. Plus it is easy to compare Travis and AppVeyor. Thoughts? |
|
I think #123 can be closed accordingly. |
The PR upgrades `cross-spawn` to `7.0.3`. The precompiled has also been updated. The `cross-spawn` version is still pinned after the PR. ----- So I have been working on improving Next.js build performance recently. One thing that catches my eye is this: <img width="1751" alt="image" src="https://user-images.githubusercontent.com/40715044/218383194-5b24c737-0d97-4434-bbbf-ba5752072882.png"> The flamegraph shows the `semver` inside `cross-spawn` is one of the hottest functions. Then I take a look at the `cross-spawn`, turns out that `cross-spawn` has `semver` already removed: moxystudio/node-cross-spawn#125 According to the CHANGELOG of `cross-spawn`, the only breaking change is that `cross-spawn@7` is dropping `Node.js < 8` support. So I assume it would be fine to upgrade `cross-spawn` to the latest version `7.0.3`, thus eliminating the extra performance overhead introduced by `semver`. --------- Co-authored-by: JJ Kasper <jj@jjsweb.site>
No description provided.