hotfix: with nodeAdapter we should get fetch from globalThis

[stalkerg]

Base on #1784 issue. Basically, under real nodejs we have no direct access to fetch even if it's set to globalThis.

[changeset-bot]

⚠️ No Changeset found

Latest commit: bb0d2db

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

[GrygrFlzr]

What about the fetch call inside serverFetch? Does that also need to be scoped with globalThis., or does this only affect the default case of not having serverFetch defined?

[stalkerg]

no, it's fallback, serverFetch is optional.

[stalkerg]

or does this only affect the default case of not having serverFetch defined?

yes.

[benmccann]

I see globalThis.fetch = fetch2; on line ~3k of the output, but there's an init function on line ~1.7k that ends up calling fetch, so I guess they've gotten put out of order somehow

It seems to be this init:

kit/packages/kit/src/core/build/index.js

Line 290 in 6ace879

export function init(settings) {

I don't quite understand how all these build pieces fit together, but in my limited understanding this PR seems to be papering over the issue2, which is that globalThis.fetch is not being set early enough. I think we may need to somehow reorder how these lines are getting put in the final code. Perhaps @Rich-Harris or @Conduitry or someone who understands this code better than I do can chime in

[benmccann]

I expect we may be hitting an incorrect assumption in esbuild: evanw/esbuild#399 (comment)

[Rich-Harris]

Oof, that's a surprising issue. This does indeed seem to be an ordering issue — anything added to globalThis can be read as a global value:

// foo.js
globalThis.foo = 42;

// index.js
import './foo.js';
console.log(foo); // 42

Is this something that broke recently?

[benmccann]

Is this something that broke recently?

@Rich-Harris, yes, it started after d729b08#diff-ed48923f0655f935bee505cdcbf40212129ff49a89c2c9d752bd63aa1e6282c4R191

[stalkerg]

@benmccann, yes, I totally agree, it's just hiding some order issue, but at the same time, it can be a temporal solution while we are trying to find and fix it.
Anyway, up to you.

[GrygrFlzr]

I just noticed, but turns out this PR actually changes the dev environment, not the build one, so it's actually modifying the wrong files to fix the issue.

Anyway, opened #1804 to try and deal with the root ordering issue rather than patch the symptoms.

[stalkerg]

I just noticed, but turns out this PR actually changes the dev environment, not the build one, so it's actually modifying the wrong files to fix the issue.

strange, at least I can't reproduce this issue anymore from #1784 after this PR.