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

chore: fix latest compile TS and Node 20 workflow failures #7512

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

JamesHenry
Copy link
Contributor

@JamesHenry JamesHenry commented Oct 29, 2024

Description:

In PR #7511 I discovered that Node 18 tests are working fine, but it looks like the latest Node 20 (20.18.0) which the Node 20 job is configured to use, has actually implemented FinalizationRegistry which was added to the tests 4 years ago (!) but has not actually been in use because of it not being present globally in Node until now.

This is obviously a very surprising discovery that this test code has not actually ever been executed in the last 4 years, and in reading the MDN entry for FinalizationRegistry (a thing I was not familiar with at all), it seems like something we'd want to avoid in deterministic tests across multiple environments.

Therefore I replaced its usage entirely with a WeakRef which should hopefully cover the desired logic on Node v20.

(https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/FinalizationRegistry)

The PR then ultimately also fixes the long standing TS compilation issues with latest TS.

Copy link

nx-cloud bot commented Oct 29, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 72884e2. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@JamesHenry
Copy link
Contributor Author

Ha, so this did indeed fix v20, but has caused v18 to fail... I will see if I can repro locally. Perhaps it just needs a few retries on the GC check...

@JamesHenry
Copy link
Contributor Author

It turns out global.gc does not exist by default in Node 18. We could either:

  • skip the tests in Node 18
  • always run node explicitly for the mocha tests and pass it the --expose-gc flag which sets global.gc. Again, it's set by default in Node 20.

I decided to go with the latter, but let me know if you would prefer the former.

@JamesHenry JamesHenry marked this pull request as ready for review October 29, 2024 14:06
@JamesHenry
Copy link
Contributor Author

JamesHenry commented Oct 29, 2024

NOTE: This will still be blocked until the type error on latest TS is resolved

Fixed in commit 72884e2

@JamesHenry JamesHenry changed the title chore: replace FinalizationRegistry usage with WeakRef to fix test on Node 20.18.0 chore: fix latest compile TS and Node 20 workflow failures Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant