fix(networking): resolve fetch lazily instead of at module scope - #353
Draft
Huxpro wants to merge 2 commits into
Draft
fix(networking): resolve fetch lazily instead of at module scope#353Huxpro wants to merge 2 commits into
fetch lazily instead of at module scope#353Huxpro wants to merge 2 commits into
Conversation
The `fetch` workaround in the networking example bound `globalThis.fetch ?? fetch` eagerly at module scope. That has two problems: - The IFR main-thread context has no `fetch` at all, so a module-scope reference crashes bundle evaluation there — the same hazard already documented in `examples/hackernews-*/src/api.ts`. - When neither binding resolves, the eager form silently stores `undefined` and surfaces as "_fetch is not a function" at the call site rather than at the point of failure. Switch to the lazy `getFetch()` resolver already used by the hackernews examples, which reads the binding at request time and throws a clear error when no `fetch` is available. The underlying lynx-stack workaround (and its TODO) is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NCNxvDyNN8HAmGtw2E5vuo
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
The comment blamed RuntimeWrapperWebpackPlugin for shadowing `fetch`
with an undefined parameter. The shadowing is deliberate: `fetch` is in
the plugin's default BOM inject list next to `window`/`document`/
`navigator`, and the generated banner emits `fetch = fetch || lynx.fetch`
as a fallback.
Both sides of that fallback are the host's to supply. On the web
platform neither is — nothing lands in the injected slot and `lynx.fetch`
is unset — so the binding resolves to `undefined`. That is a web-platform
gap against its own documented contract ("Web Platform supports Fetch API
using Browser's Fetch implementation"), not a bundler defect.
Retarget the comment and the TODO accordingly. No behaviour change.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NCNxvDyNN8HAmGtw2E5vuo
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Two changes to
examples/networking/src/App.vue, the site of the repo'sTODO: Remove once lynx-stack shims 'fetch' on the 'lynx' global.1. Resolve
fetchlazily instead of at module scopeThe example bound the workaround eagerly:
That form has two problems:
fetchat all, so a module-scope reference crashes bundle evaluation there. This hazard is already documented — and already solved — inexamples/hackernews-css/src/api.tsandexamples/hackernews-tailwind/src/api.ts. The networking example never picked up the fix. (enableIFRdefaults tofalseand this example doesn't set it, so nothing is broken today; this makes the example correct if IFR is turned on.)undefined, and the failure surfaces as_fetch is not a functionat whichever call site runs first, rather than wherefetchwas found to be missing.Replaced with the lazy
getFetch()resolver already used by the hackernews examples — same shape, verbatim — and updated the three call sites.2. Correct the attribution in the comment
The original comment blamed the bundler:
That misplaces the defect. The shadowing is deliberate —
fetchsits inRuntimeWrapperWebpackPlugin's default BOM inject list right next towindow,document,navigator,localStorage— and the generated banner ships an explicit fallback:Both sides of that fallback are the host's to supply. On the web platform neither is: nothing lands in the injected slot and
lynx.fetchis unset, so the binding resolves toundefined.globalThis.fetchis untouched and still reaches the browser's real implementation, which is why the workaround works.So this is a web-platform gap against its own documented contract — the Lynx docs state that
fetchis a global (notlynx.fetch) and that "Web Platform supports Fetch API using Browser's Fetch implementation, it follows the same rules as the web" — not a bundler defect. The comment and the TODO now point there.Verification
RuntimeWrapperWebpackPlugininject list and banner: read from source, quoted above.fetchcontract: from the LynxfetchAPI docs.lynxCoreInjectwas not read, so which package drops the injection — and whether current versions still do — is inference, not confirmed. The workaround is left in place regardless.Testing
Workspace dependencies aren't installed in this environment, so the example's
vue-tsctypecheck could not be run. The extracted<script setup>block parses clean (node --experimental-strip-types --check), and change 1 is a like-for-like port of a pattern already shipping in two other examples. Change 2 is comments only.Notes on the other TODOs
The repo has exactly three first-party TODO comments; the other two are not actionable here:
packages/vue-lynx/main-thread/src/shims.d.ts:28— asks for a fix in@lynx-js/type-element-api(a different repo). Worth flagging: applying its recommendedElementRef | ElementRef[]union to the local override would be a regression, since the narrow array-only declaration is what stops callers from passing a bare element and crashing on the web PAPI.packages/vue-lynx/runtime/src/node-ops.ts:51— blocked on a Lynx engine fix for numericflexthrough__SetInlineStyles; not verifiable without the engine.Follow-up worth considering (not in this PR):
examples/ai-chatcarries the same eagerconst _fetch = globalThis.fetch ?? fetchin three separate files (src/lib/api.ts,src/lib/backend-mode.ts,src/lib/stream.ts), each with the same misattributing comment.🤖 Generated with Claude Code
https://claude.ai/code/session_01NCNxvDyNN8HAmGtw2E5vuo