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

build: add node v18 support #7812

Merged
merged 4 commits into from Apr 22, 2022
Merged

build: add node v18 support #7812

merged 4 commits into from Apr 22, 2022

Conversation

Shinigami92
Copy link
Member

@Shinigami92 Shinigami92 commented Apr 19, 2022

Description

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@Shinigami92 Shinigami92 self-assigned this Apr 19, 2022
@Shinigami92 Shinigami92 added enhancement New feature or request p2-nice-to-have Not breaking anything but nice to have (priority) labels Apr 19, 2022
@Shinigami92 Shinigami92 added this to the 3.0 milestone Apr 19, 2022
@Shinigami92
Copy link
Member Author

Note for myself: need to update

"@types/node": "^16.11.27",

@@ -37,7 +37,7 @@
"@microsoft/api-extractor": "^7.22.2",
"@types/fs-extra": "^9.0.13",
"@types/jest": "^27.4.1",
"@types/node": "^16.11.27",
"@types/node": "^17.0.25",
Copy link
Member Author

Choose a reason for hiding this comment

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

Awaiting v18 types

Copy link
Member Author

Choose a reason for hiding this comment

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

@Shinigami92 Shinigami92 mentioned this pull request Apr 20, 2022
9 tasks
@Shinigami92 Shinigami92 changed the title build: upgrade to node 18 build!: upgrade to node 18 Apr 20, 2022
@Shinigami92 Shinigami92 changed the title build!: upgrade to node 18 build: upgrade to node 18 Apr 20, 2022
@Shinigami92 Shinigami92 changed the title build: upgrade to node 18 build: add node v18 support Apr 20, 2022
Copy link
Member

@sodatea sodatea left a comment

Choose a reason for hiding this comment

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

It's due to this breaking change: nodejs/node#41431

.filter((detail) => detail && detail.address && detail.family === 'IPv4')

In Node.js < 18, detail.family is a string; in Node.js >= 18, it's an integer.

@Shinigami92
Copy link
Member Author

I'm not 💯 % sure, but maybe the failing node-18-windows job is a jest node-18 globalTeardown bug?

@Shinigami92 Shinigami92 marked this pull request as ready for review April 22, 2022 14:18
@sodatea
Copy link
Member

sodatea commented Apr 22, 2022

About the failing test on WIndows:

  • I don't see anything wrong with the specific test case except it's the last one that gets executed.
  • So I think, even if it's a real issue, it's likely to be a bug with Jest and Node.js 18, we can safely ignore it…
  • …until someone with a Windows machine can actually help debug it.

@sodatea
Copy link
Member

sodatea commented Apr 22, 2022

Done a little research and found a possible culprit: jprichardson/node-fs-extra#646

Now that we also have rimraf in dependencies, maybe we can replace the fs.removeSync in jestGlobalTeardown.cjs with rimraf and see what happens?

Related commits:

rimraf retries 100 times; fs-extra retries for 500ms.
So rimraf is likely to fail on very fast machines and fs-extra may fail on slow machines…

@Shinigami92
Copy link
Member Author

Shinigami92 commented Apr 22, 2022

Done a little research and found a possible culprit: jprichardson/node-fs-extra#646

Now that we also have rimraf in dependencies, maybe we can replace the fs.removeSync in jestGlobalTeardown.cjs with rimraf and see what happens?

Related commits:

rimraf retries 100 times; fs-extra retries for 500ms. So rimraf is likely to fail on very fast machines and fs-extra may fail on slow machines…

I may do this this evening or weekend in a separate PR, need to be "afk" for a while
In the meantime, I would suggest that we already merge this PR as it fixes already a compatibility log bug 🙂

@Shinigami92 Shinigami92 mentioned this pull request Apr 22, 2022
8 tasks
@patak-dev patak-dev merged commit fc89057 into main Apr 22, 2022
@patak-dev patak-dev deleted the node-18 branch April 22, 2022 19:20
@patak-dev
Copy link
Member

@Shinigami92 I see that this was marked for 3.0, this is ok to be released in 2.9 though, no?

@patak-dev patak-dev removed this from the 3.0 milestone Apr 24, 2022
@Shinigami92
Copy link
Member Author

Yes, I marked it with 3.0 as I also deprecated v12 in one PR
Now I split them into separate 👍

@Shinigami92 Shinigami92 mentioned this pull request Apr 24, 2022
7 tasks
@bluwy bluwy mentioned this pull request May 13, 2022
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request on hold p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants