fix(node): Node 10 support #5769
Conversation
|
|
We should remove |
|
@imsnif have you updated Yarn's usage of Unless there's a problematic edge case I'm not aware of, you could probably do a quick find/replace and include that commit in this PR 😄 |
|
@leosco this was already done. |
|
When will this move to a release? |
|
@matt3224 once we figure out the issue with Windows builds, we'll merge this PR and release a new version. |
|
@BYK any news? 🙏 |
|
|
|
@BYK @aracarie - Can we clear the AppVeyor build cache and rerun this build? 🤞🙏 I don't have a Windows machine, otherwise I would help! |
…arnpkg#5793) * fixes yarnpkg#5755 * lint and duplication fixes * fix test
|
Good news everyone! I've traced the issue down to a change in how |
There was a problem hiding this comment.
Just a couple minor comments, but overall looks good.
Ran tests locally on OSX node v8 and v10. Ran yarn cache clear && yarn install against the most complex project I have available (>1,000 total packages get installed, and includes some github urls and resolutions) and it worked.
| @@ -1,2 +0,0 @@ | |||
| # this file will be generated | |||
| index.js | |||
There was a problem hiding this comment.
was there a reason to stop ignoring this file? I assume git won't see any changes between test runs, just seemed unrelated to the node10 changes.
There was a problem hiding this comment.
Yes. This file was dynamically written right before two test runs which I believe was responsible for the test failures on macOS. See 5392392 for the full change related to this.
| }); | ||
| }); | ||
|
|
||
| test.concurrent('should install if symlink source does not exist', async (): Promise<void> => { |
There was a problem hiding this comment.
was this test removed as part of a behavior change? From the package.json in this test's fixture, it's hard to tell what was actually being tested...
There was a problem hiding this comment.
oh nevermind, I see this was part of a commit "remove redundant test"
|
fyi, I added a couple "fixes" references to the original PR text so we can get a couple node10 specific issues closed out when this merges. |
Summary
net.createServermethod. This was having issues, likely because of this: stream: 'end' is no longer emitted after 'error' nodejs/node#20334 - it now uses a "normal"httpserver instead, serving the same purpose.readFirstAvailableStreammethod inutil/fs.js. I'm guessing this is related to the same issue as 2, but there have been quite some changes in10that might be causing this - so it's just a guess. Since this piece of code could be clearer and more conceise anyway, I opted to just change it. Now instead of relying on the streamerrorevent to check if a file exists, it just explicitly checks it before starting the stream.upathfrom1.0.2to1.0.5which supports node 10 (found through itsenginesfield when installing).fixes #5761
fixes #5477
Test plan
CI should be green (including and especially the new node10 job!)