ReactiveX / rxjs

A reactive programming library for JavaScript
https://rxjs.dev
Apache License 2.0
30.81k stars 3.01k forks source link

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

Open JamesHenry opened 3 weeks ago

JamesHenry commented 3 weeks ago

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.

nx-cloud[bot] commented 3 weeks ago

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 72884e22b93aab8e58e12e2e8b90d24748f469c0. 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 - [`nx compile rxjs`](https://cloud.nx.app/runs/a7GLPTuVdv?utm_source=pull-request&utm_medium=comment)

Sent with 💌 from NxCloud.

JamesHenry commented 3 weeks ago

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 commented 3 weeks ago

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

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

JamesHenry commented 3 weeks ago

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

Fixed in commit https://github.com/ReactiveX/rxjs/pull/7512/commits/72884e22b93aab8e58e12e2e8b90d24748f469c0