-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Add config.kit.prerender.origin, enable fetching from local endpoints during prerendering
#5627
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: 37affe5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Co-authored-by: Simon H <[email protected]>
Co-authored-by: Rich Harris <[email protected]>
dummdidumm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two minor questions, but nothing that blocks me from approving this
| Object.defineProperty(globalThis, name, { | ||
| enumerable: true, | ||
| configurable: true, | ||
| writable: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will make all globals writable - could this have unintended side effects, like people starting to monkey patch this stuff when they shouldn't? Should we limit this to fetch, or is it actually better that people have the ability to override if needed (I'd say yes, but I wanted to bring this up).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I had the same internal conversation. I decided that since we're able to write these values, it's only fair that others can too — there are always going to be legitimate reasons why someone might want to intercept one of these things (if only for debugging)
| export default config; | ||
| ``` | ||
|
|
||
| - `origin` — the value of `url.origin` during prerendering; useful if it is included in rendered content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make it more apparent that this is from the $page store and/or link to it?
| - `origin` — the value of `url.origin` during prerendering; useful if it is included in rendered content | |
| - `origin` — the value of [`$page.url.origin`](/docs/modules#$app-stores-page) during prerendering; useful if it is included in rendered content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The trouble is it's not just $page, it's the url property of RequestEvent and LoadEvent too, so I figured url.origin was the most catch-all. Most people will never need to touch this setting anyway so it probably doesn't matter all that much
Fixes #5444
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. All changesets should bepatchuntil SvelteKit 1.0