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

[takurinton]

I've defined globalThis.fetch at https://github.com/preactjs/wmr/pull/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?

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: 7c1bb6ca93748cdb2f1f51eeab521e9b0808c647

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]

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

[takurinton]

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

[takurinton]

@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..)

[rschristian]

@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]

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]

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]

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]

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]

@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]

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]

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]

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]

@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]

Seems pretty stable to me, see #938.

Definitely not from the fetch impl

[takurinton]

@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]

All tests passed!!!