fix: Externalize app initialization to adapters - #1804
Merged
Conversation
🦋 Changeset detectedLatest commit: 8346eb1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
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 |
GrygrFlzr
commented
Jul 2, 2021
Comment on lines
-4
to
+3
| globalThis.fetch = fetch; | ||
| globalThis.Response = Response; | ||
| globalThis.Request = Request; | ||
| globalThis.Headers = Headers; | ||
| Object.defineProperties(globalThis, { |
Member
Author
There was a problem hiding this comment.
defineProperties seems like a more robust solution, and also avoids having to slap ts-nocheck directives.
GrygrFlzr
commented
Jul 2, 2021
| @@ -1,2 +1,2 @@ | |||
| import { createRequire } from 'module'; | |||
| global.require = createRequire(import.meta.url); | |||
| globalThis.require = createRequire(import.meta.url); | |||
Member
Author
There was a problem hiding this comment.
Consistency with the rest of the codebase.
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.
Fixes #1784. Closes #1786 as its alternative.
This PR contains a breaking change to the adapter API.
Current Behavior
kit/packages/kit/src/core/build/index.js
Line 383 in 559aad5
The built
app.jsfile contains a side effect of calling theinitfunction, which ends up running before polyfills like@sveltejs/kit/install-fetch. Although ESM (supposedly) guarantees a top-down order for import side effects, this is currently not necessarily true with esbuild nor rollup (see evanw/esbuild#399 (comment)).Proposed Behavior
kit/packages/kit/src/core/build/index.js
Lines 288 to 292 in e2f1b3f
The
initcall is removed from the builtapp.js, and instead it becomes the responsibility of the adapters to call it at least once. This lets it control when the initialization happens, allowing us to guarantee that it happens after any polyfills:kit/packages/adapter-node/src/index.js
Lines 4 to 8 in e2f1b3f
This may also be useful in the future should the
initfunction need to perform other side effects.Non-breaking alternate solutions
require()and the require shim to import the app to force it to run last:initfromrenderthe first time it's called:adapter-nodeit won't be evaluated until the server handles the quest, which may delay the initial response. May also affect serverless environments where lambdas may live longer than a single request.