Feat proposal enable fetching external resources using native fetch API in Node.js version 18 later #935

takurinton · 2022-09-18T07:23:00Z

I've defined globalThis.fetch at #934, but currently wmr can't access external resources when use prerender mode.
Therefore, I came up with a plan to make fetch available natively in Node.js 18 and later.

But Node.js 18 is not LTS yet, what do you think?

…version 18 later

changeset-bot · 2022-09-18T07:23:03Z

🦋 Changeset detected

Latest commit: 7c1bb6c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
wmr	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

takurinton · 2022-09-18T07:24:28Z

packages/wmr/test/production.test.js

+
+			const nodeVersion = process.version;
+			const isNodeVersionOver18 = Number(nodeVersion.split('.')[0].slice(1)) >= 18;
+			if (isNodeVersionOver18) {


Is there a way to run tests on multiple versions?

Oh, and two versions, 12 and 14, are running now. This felt like a no problem in the future.

I'm also considering breaking down this test.

Instead of branching using if in one test, write a test for Node.js version 17 or less and a test for Node.js version 18 or more, and skip if the version is not applicable. I think it is also possible.
This part is one of the things I want to discuss in this PR.

Instead of branching using if in one test, write a test for Node.js version 17 or less and a test for Node.js version 18 or more, and skip if the version is not applicable.

I like this idea. There's slightly different behavior between the two that (ideally) we'd test.

We can definitely alter the CI config to cover newer versions of Node.

I completely forgot about this.
Last time I called fetch without the leading slash here, but this test does not work with the regex changes you reviewed.
https://github.com/preactjs/wmr/blob/main/packages/wmr/test/fixtures/prerender-resource-fetch/index.js#L2

I think to need to modify the test here as well. I think the comments here will be helpful for that.
#935 (comment)

Oops, no correction of above comment was necessary.

…ive `fetch`

packages/wmr/src/lib/prerender.js

…alLink to isLocalFile

takurinton · 2022-09-22T06:08:05Z

TODO: will fix test
I'm going to work!
I will write this PR when we come back!

…iles in Node.js 18 later

takurinton · 2022-09-22T19:23:15Z

packages/wmr/src/lib/prerender.js

+		// @ts-ignore
+		globalThis.fetch = async (url, options) => {
+			if (pattern.test(String(url))) {
+				return localFetcher(url);
 			}

-			console.warn(`fetch is not implemented in Node.js, please upgrade to Node.js 18 or above`);
+			// When fetching an external resource, it returns the native fetch API
+			// though `fetcher` function is an alias created to avoid infinite loops
+			return fetcher(url, options);


overrided globalThis.fetch with our own implementation.
ref #935 (comment)

The fetcher function is an alias function defined to avoid infinite loops and is equivalent to the native fetch function.

When Node.js version 18 later, I addressed this by writing conditional expressions for both fetch is defined and it is undefined to override.
I also wrote corresponding tests.

Local fetching should now be automated. If you expect a different behavior from the fetch that wmr itself is overriding, you should be able to solve this by writing the following code.

export async function prerender(vnode: VNode) { globalThis.funkyFetch = ... } const fetcher = typeof window === 'undefined' ? funkyFetch : fetch; fetcher(...);

packages/wmr/test/fixtures/prerender-resource-fetch/index.js

takurinton · 2022-09-22T20:57:14Z

After merging 936 into main, follow this branch as well and run the test

rschristian · 2022-09-22T21:12:16Z

packages/wmr/src/lib/prerender.js

+		// When Node.js version under 17, native fetch API undefined
+		// So it will give a warning when the syntax to retrieve the file and the url is passed
+		// @ts-ignore
+		globalThis.fetch = async url => {
+			if (pattern.test(String(url))) {
+				return localFetcher(url);
+			}
+
+			console.warn(`fetch is not defined in Node.js version under 17, please upgrade to Node.js version 18 later`);
+		};
+	} else {
+		// At that time, implement using reassignment to avoid entering an infinite loop.
+		const fetcher = fetch;
+		// @ts-ignore
+		delete globalThis.fetch;
+
+		// When Node.js version 18 later, native fetch API is defined
+		// Override with own implementation if you want to retrieve local files in Node.js 18 later
+		// ref https://github.com/preactjs/wmr/pull/935#discussion_r977168334
+		// @ts-ignore
+		globalThis.fetch = async (url, options) => {
+			if (pattern.test(String(url))) {
+				return localFetcher(url);
+			}
+
+			// When fetching an external resource, it returns the native fetch API
+			// though `fetcher` function is an alias created to avoid infinite loops
+			return fetcher(url, options);


The comments explaining the workflow here are excellent! Really good stuff.

takurinton · 2022-09-22T21:26:55Z

@rschristian
Currently the tests running on CI are failing, but I have tried locally on my own and they seem to be unstable as well.
Does this work in your environment??

(We've talked about tests being unstable before..)

packages/wmr/test/production.test.js

rschristian · 2022-09-22T21:31:07Z

@takurinton Hmm yeah that's a weird error in the CI. Will look into it. Not sure if it's unstable or just something odd has broken.

Are you using 18 locally? Do the tests sometimes pass?

takurinton · 2022-09-22T21:33:15Z

Are you using 18 locally?

yes, I am using v18.9.0

% node -v
v18.9.0

Do the tests sometimes pass?

Only the ● production 'demo app' should serve the demo app test fails every time. Other than that, it works sometimes.

rschristian · 2022-09-22T21:35:43Z

Yeah that's the really weird error. Can't find the entry module, wonder if the resolution changed or something in Node 18.

Thanks for the extra info, that help me later. We won't block your PR on that working though, as it's outside your changes here so don't worry about those failures.

takurinton · 2022-09-22T21:41:23Z

test name fiexd!!!

Thanks for the extra info, that help me later. We won't block your PR on that working though, as it's outside your changes here so don't worry about those failures.

Thanks!!! I'm relieved it's outside the scope of my implementation this time around!
But I'm curious about this issue and will work on it when I have time!

rschristian

Looks good to me, thanks!

rschristian · 2022-09-23T08:26:22Z

I'm not confident this is a test issue we can ignore. Earlier I misinterpreted one of our tests as being an error when it wasn't. Haven't used this in so long I forgot what our tests were doing.

The tests pass on the main branch and they're now running with Node 18 due to your earlier PR. If you are in fact seeing a test fail every time, I think it's an issue with this PR.

takurinton · 2022-09-23T08:31:40Z

@rschristian
The test on the main branch is dry run because there are no changes in the actual code.
I think the way to find out if this is really a problem with this branch is to rerun the test in another PR or create a new one to see if this is the case.
What do you think?

rschristian · 2022-09-23T08:34:13Z

Oh shoot. Yep, you're right. Sorry about that.

I'll spend some time tracking this down when I can. Would like to cut a release afterwards but wouldn't want to do that if there might be issues (related to this or not).

takurinton · 2022-09-23T08:48:02Z

I am also trying to fix a few things about my demo app test failing.
Locally it fails because the .cache directory does not exist at my end, but in CI it fails because the dist directory does not exist.
And while I can just ignore the .cache directory with error handling if it doesn’t exist, I can’t proceed to the next process if I can’t get the dist directory.

For example, I can ignore the absence of a .cache directory by writing code like this in my local tests. (However, this is not where CI fails, so it is practically a meaningless workaround.)

await loadFixture('../../../../examples/demo', env);
// Often an error occurs here where the directory does not exist
// If the directory does not exist, skip `rmdir` and continue
for (const d of ['dist', 'node_modules', '.cache']) {
	try {
		await fs.rmdir(path.join(env.tmp.path, d), { recursive: true });
	} catch (e) {
		console.error(`skip removing. Because ${e.message}`);
	}
}

The actual log of the test looks like this.

local test

● production › demo app › should serve the demo app
ENOENT: no such file or directory, stat ‘/path/to/demo/.cache’

CI test

● production › demo app › should serve the demo app
ENOENT: no such file or directory, stat ‘/tmp/tmp-2076-LY3Agd84WR82/demo/dist’

This test is not compatible or reproducible locally and on CI, very difficult...

takurinton · 2022-09-23T08:52:39Z

I'll spend some time tracking this down when I can. Would like to cut a release afterwards but wouldn't want to do that if there might be issues (related to this or not).

Thank you.
I agree with that. It is very difficult to find a drop-off point.
The test was unstable to begin with, but I am afraid that this release will have an impact.

takurinton · 2022-09-23T09:38:43Z

@rschristian
I created another PR and ran the test on GitHub Actions and it failed in the same way.
I still can't be certain that the fetch change has no impact, but at least it appears to have no impact.

You can see log here.
https://github.com/preactjs/wmr/actions/runs/3111711765/jobs/5044311973

rschristian · 2022-09-24T01:25:17Z

Seems pretty stable to me, see #938.

Definitely not from the fetch impl

takurinton · 2022-09-25T02:23:13Z

@rschristian
I also feel that the fetch implementation is fine, as I wrote an exception in the fs.rmdir location in #937 and it stabilized!
#937 has been closed

takurinton · 2022-09-26T16:26:45Z

All tests passed!!!

Enable fetching external resources using native fetch API in Node.js …

3473559

…version 18 later

takurinton commented Sep 18, 2022

View reviewed changes

fix: remove fetch after redefinition to avoid infinite loop

c9d9d56

takurinton changed the title ~~WIP: enable fetching external resources using native fetch API in Node.js version 18 later~~ Enable fetching external resources using native fetch API in Node.js version 18 later Sep 18, 2022

fix: when Node.js version is only over 18 define _fetch instead nat…

aa0818d

…ive `fetch`

takurinton marked this pull request as ready for review September 18, 2022 20:21

takurinton added 2 commits September 19, 2022 13:31

add: changeset

8acd946

fix

74f90a9

rschristian reviewed Sep 22, 2022

View reviewed changes

packages/wmr/src/lib/prerender.js Outdated Show resolved Hide resolved

packages/wmr/src/lib/prerender.js Outdated Show resolved Hide resolved

packages/wmr/src/lib/prerender.js Outdated Show resolved Hide resolved

fix: globalThis.fetch only calls when fetch is undefined and isExtern…

54e5d2a

…alLink to isLocalFile

takurinton added 2 commits September 22, 2022 21:46

fix: override with own implementation if you want to retrieve local f…

27def4e

…iles in Node.js 18 later

fix: tests

f1a51dd

takurinton commented Sep 22, 2022

View reviewed changes

takurinton changed the title ~~Enable fetching external resources using native fetch API in Node.js version 18 later~~ Feat proposal enable fetching external resources using native fetch API in Node.js version 18 later Sep 22, 2022

takurinton commented Sep 22, 2022

View reviewed changes

packages/wmr/test/fixtures/prerender-resource-fetch/index.js Show resolved Hide resolved

takurinton mentioned this pull request Sep 22, 2022

Fix Node.js version of GitHub Actions workflow #936

Merged

Merge branch 'main' into add-fetch-for-node-18

38b7dd5

rschristian reviewed Sep 22, 2022

View reviewed changes

packages/wmr/test/production.test.js Outdated Show resolved Hide resolved

packages/wmr/test/production.test.js Outdated Show resolved Hide resolved

packages/wmr/test/production.test.js Outdated Show resolved Hide resolved

packages/wmr/test/production.test.js Outdated Show resolved Hide resolved

fix: test names

63954dd

takurinton requested a review from rschristian September 22, 2022 22:16

rschristian approved these changes Sep 22, 2022

View reviewed changes

takurinton mentioned this pull request Sep 23, 2022

Investigation of tests failing due to Node.js version 18 #937

Closed

developit approved these changes Sep 26, 2022

View reviewed changes

Merge branch 'main' into add-fetch-for-node-18

7c1bb6c

developit approved these changes Sep 26, 2022

View reviewed changes

rschristian merged commit e546afc into preactjs:main Sep 26, 2022

This was referenced Sep 26, 2022

Version Packages #877

Merged

Version Packages takurinton/wmr#1

Open

takurinton mentioned this pull request Oct 15, 2022

render external resource OG takurinton/blog.takurinton.dev#23

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat proposal enable fetching external resources using native fetch API in Node.js version 18 later #935

Feat proposal enable fetching external resources using native fetch API in Node.js version 18 later #935

takurinton commented Sep 18, 2022 •

edited

Loading

changeset-bot bot commented Sep 18, 2022 •

edited

Loading

takurinton Sep 18, 2022

takurinton Sep 18, 2022 •

edited

Loading

takurinton Sep 21, 2022

rschristian Sep 22, 2022

takurinton Sep 22, 2022

takurinton Sep 22, 2022 •

edited

Loading

takurinton commented Sep 22, 2022 •

edited

Loading

takurinton Sep 22, 2022

takurinton Sep 22, 2022

takurinton Sep 22, 2022 •

edited

Loading

takurinton commented Sep 22, 2022

rschristian Sep 22, 2022

takurinton commented Sep 22, 2022

rschristian commented Sep 22, 2022

takurinton commented Sep 22, 2022

rschristian commented Sep 22, 2022

takurinton commented Sep 22, 2022

rschristian left a comment

rschristian commented Sep 23, 2022

takurinton commented Sep 23, 2022

rschristian commented Sep 23, 2022

takurinton commented Sep 23, 2022

takurinton commented Sep 23, 2022

takurinton commented Sep 23, 2022

rschristian commented Sep 24, 2022

takurinton commented Sep 25, 2022

takurinton commented Sep 26, 2022

Feat proposal enable fetching external resources using native fetch API in Node.js version 18 later #935

Feat proposal enable fetching external resources using native fetch API in Node.js version 18 later #935

Conversation

takurinton commented Sep 18, 2022 • edited Loading

changeset-bot bot commented Sep 18, 2022 • edited Loading

🦋 Changeset detected

takurinton Sep 18, 2022

Choose a reason for hiding this comment

takurinton Sep 18, 2022 • edited Loading

Choose a reason for hiding this comment

takurinton Sep 21, 2022

Choose a reason for hiding this comment

rschristian Sep 22, 2022

Choose a reason for hiding this comment

takurinton Sep 22, 2022

Choose a reason for hiding this comment

takurinton Sep 22, 2022 • edited Loading

Choose a reason for hiding this comment

takurinton commented Sep 22, 2022 • edited Loading

takurinton Sep 22, 2022

Choose a reason for hiding this comment

takurinton Sep 22, 2022

Choose a reason for hiding this comment

takurinton Sep 22, 2022 • edited Loading

Choose a reason for hiding this comment

takurinton commented Sep 22, 2022

rschristian Sep 22, 2022

Choose a reason for hiding this comment

takurinton commented Sep 22, 2022

rschristian commented Sep 22, 2022

takurinton commented Sep 22, 2022

rschristian commented Sep 22, 2022

takurinton commented Sep 22, 2022

rschristian left a comment

Choose a reason for hiding this comment

rschristian commented Sep 23, 2022

takurinton commented Sep 23, 2022

rschristian commented Sep 23, 2022

takurinton commented Sep 23, 2022

takurinton commented Sep 23, 2022

takurinton commented Sep 23, 2022

rschristian commented Sep 24, 2022

takurinton commented Sep 25, 2022

takurinton commented Sep 26, 2022

takurinton commented Sep 18, 2022 •

edited

Loading

changeset-bot bot commented Sep 18, 2022 •

edited

Loading

takurinton Sep 18, 2022 •

edited

Loading

takurinton Sep 22, 2022 •

edited

Loading

takurinton commented Sep 22, 2022 •

edited

Loading

takurinton Sep 22, 2022 •

edited

Loading