-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
feat: implement read
#11649
feat: implement read
#11649
Conversation
🦋 Changeset detectedLatest commit: 2b3746c The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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 |
Should it be the responsibility of each adapter's |
…nt appear to respect .netlify directory
I think that probably falls under 'play stupid games, win stupid prizes'. I suppose we could check the input against the asset list (which gets shipped with the app anyway IIRC). Another thought: if we did do that, we could make |
Oh, I like the idea of |
Implemented that idea. I also changed the API as a result — if Kit already knows the length and type, there's no point passing that information to the adapter-provided As a bonus, we're now able to throw an error if There's some room for improvement in the implementation but I feel good about this direction. |
} | ||
|
||
if (manifest_data.hooks.server) { | ||
// TODO if hooks.server.js imports `read`, it will be in the entry chunk |
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.
do we need a dev time warning here or would it be ok to be in the entry chunk?
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.
Due to some quirk of how the resulting Vite bundle is structured (or something like that) I couldn't actually get this to work reliably, so I kicked the can down the road for now. The reason I felt comfortable doing that is that if you used read
inside handle
you would get a dev time error on every request (and if it was in hooks.server.js
outside handle then the app wouldn't start in the first place), so the chance of this causing an unexpected error in prod is effectively zero
@@ -1159,6 +1178,8 @@ export interface SSRManifest { | |||
nodes: SSRNodeLoader[]; | |||
routes: SSRRoute[]; | |||
matchers(): Promise<Record<string, ParamMatcher>>; | |||
/** A `[file]: size` map of all assets imported by server code */ | |||
server_assets: Record<string, number>; |
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.
if someone used a glob import, would this potentially turn into a very large list? do we actually need all filenames if only few might ever be passed through read
?
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.
Yes but this is just a caveat of using import.meta.glob
anyway — you will always end up with filenames written into the resulting bundle, so you'd better make sure you're only globbing things that you will actually use
@@ -289,7 +289,7 @@ importers: | |||
dependencies: | |||
'@sveltejs/kit': | |||
specifier: ^1.0.0 || ^2.0.0 | |||
version: link:../kit | |||
version: 2.3.5(@sveltejs/[email protected])([email protected])([email protected]) |
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.
hmm. this is weird
Co-authored-by: Ben McCann <[email protected]>
Co-authored-by: Ben McCann <[email protected]>
This implementation has some limitations:
I made a this gist as a work-around, which can be used for inspiration of a more flexible implementation in SvelteKit: https://gist.github.com/gitaarik/59213791bc2e10977de81d90ac5a0fdb |
This implements the
read(file: string): Response
idea proposed in #11647 (comment).Adapters can provide an implementation when they call
server.init
:This also adds a
read
function (name bikesheddable) in@sveltejs/kit/node
that turns a filepath into aResponse
, meaning that Node-like adapters have an easy job (see theadapter-node
example in this PR).Files that fall below the
assetsInlineLimit
are also handled (adapters don't need to provide an implementation, it's handled inside Kit).TODO:
adapter-netlify
andadapter-vercel
)paths.base
hooks.server.js
$app/server
can only be imported by server coderead
from$app/server
is used, but adapter provides noread
implementationPlease don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.Edits