Skip to content

Conversation

sebastianquek
Copy link
Contributor

@sebastianquek sebastianquek commented Feb 17, 2023

Tested with the following Node.js versions. These versions were installed using nvm (e.g. nvm install 7).

  • v7.10.1
  • v8.17.0
  • v9.11.2
  • v10.24.1
  • v11.15.0
  • v12.22.12
  • v13.14.0
  • v14.21.1
  • v15.14.0
  • v16.19.1
  • v17.9.1
  • v18.14.1
  • v19.7.0

Changes

Most of the changes are related to removing the use of modern node features or by relying on a polyfill instead.

  • Replace AbortSignal.timeout(timeout) with a setTimeout
  • Replace Object.fromEntries with a for-loop
  • Use fetch from cross-fetch@3.1.4 (rather than Node's in-built fetch)
  • Polyfill globalThis, URL, URLSearchParams using core-js-pure@3.28.0

Misc changes

  • Update examples to be more catered for different node versions
  • Remove tests in the npm library.

Related to #6

@sebastianquek sebastianquek self-assigned this Feb 17, 2023
@sebastianquek sebastianquek force-pushed the widen-node-support branch 3 times, most recently from aacff5b to 36f6ba6 Compare February 24, 2023 11:47
@aciddjus
Copy link
Contributor

aciddjus commented Mar 1, 2023

Can we add tests in GitHub actions for different Node versions?

@sebastianquek
Copy link
Contributor Author

@aciddjus Yes, that’s possible. Given we’ve manually tested it in our pairing session, what do you think if we add automated tests in a separate PR?

@aciddjus
Copy link
Contributor

aciddjus commented Mar 1, 2023

Since tests would be directly related to what the PR is addressing let's have them here.
Manually testing things is fine, but we should almost never rely just on that when pushing to production.

@sebastianquek
Copy link
Contributor Author

I've modified the approach to handle polyfills. Now if the runtime does not have native implementations of the functions/classes (i.e. fetch, globalThis, URL, URLSearchParams), then the associated polyfill is dynamically imported and used. This creates a robust way to support runtimes that already have a native implementation.

It opens up the possibility of supporting new runtimes that do not support most Node.js APIs such as Vercel Edge functions and Cloudflare Workers: #12

I've also temporarily disabled the home depot tests as that API is failing.

sebastianquek and others added 2 commits March 16, 2023 18:15
We are not targeting browsers, we don't need polyfills for them.
This also removes extra dependencies, which improves compatibility.
@zyc9012 zyc9012 force-pushed the widen-node-support branch 4 times, most recently from d084de7 to c29e4bd Compare April 17, 2023 08:16
@zyc9012 zyc9012 force-pushed the widen-node-support branch 2 times, most recently from b42ac6f to 7cdf8c5 Compare April 17, 2023 09:17
@zyc9012 zyc9012 force-pushed the widen-node-support branch 2 times, most recently from 91e08c1 to 8c2f0d9 Compare April 17, 2023 10:12
@zyc9012 zyc9012 force-pushed the widen-node-support branch from 8c2f0d9 to 8d37edb Compare April 17, 2023 10:43
@zyc9012
Copy link
Collaborator

zyc9012 commented Apr 17, 2023

As we are not targeting browsers, we do not need to polyfill the missing APIs. The new commits by me have removed all the dependencies for the nodejs package. Built-in Node modules is not introducing new deps on the deno side either.

Before:
image

After:
image

@zyc9012 zyc9012 force-pushed the widen-node-support branch from 05a4bed to 6651ddd Compare May 22, 2023 09:25
@zyc9012 zyc9012 force-pushed the widen-node-support branch from 6651ddd to 16d4113 Compare May 22, 2023 09:31
@zyc9012 zyc9012 force-pushed the widen-node-support branch from 16d4113 to 545da21 Compare May 22, 2023 09:37
@zyc9012 zyc9012 force-pushed the widen-node-support branch from e8ba809 to 99fc921 Compare May 22, 2023 11:28
Copy link
Contributor

@aciddjus aciddjus left a comment

Choose a reason for hiding this comment

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

Thank you, @zyc9012 💯
LGTM!

@zyc9012 zyc9012 merged commit e4f71bb into master May 24, 2023
@zyc9012 zyc9012 deleted the widen-node-support branch May 24, 2023 11:03
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.

3 participants