Skip to content
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

Merged
merged 59 commits into from
Jan 18, 2024
Merged

feat: implement read #11649

merged 59 commits into from
Jan 18, 2024

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Jan 16, 2024

This implements the read(file: string): Response idea proposed in #11647 (comment).

Adapters can provide an implementation when they call server.init:

server.init({
  env,
  read: (file) => new ReadableStream(...)
});

This also adds a read function (name bikesheddable) in @sveltejs/kit/node that turns a filepath into a Response, meaning that Node-like adapters have an easy job (see the adapter-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:

  • implement for other adapters (namely adapter-netlify and adapter-vercel)
  • docs
  • bikeshed the name
  • figure out how this all interacts with paths.base
  • add server assets imported by hooks.server.js
  • ensure $app/server can only be imported by server code
  • throw during dev/build if read from $app/server is used, but adapter provides no read implementation

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Edits

  • Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

Copy link

changeset-bot bot commented Jan 16, 2024

🦋 Changeset detected

Latest commit: 2b3746c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@sveltejs/adapter-netlify Major
@sveltejs/adapter-vercel Major
@sveltejs/adapter-node Major
@sveltejs/kit Minor

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

@Conduitry
Copy link
Member

Should it be the responsibility of each adapter's readAsset function to check that it's not being asked to read some potentially sensitive file that lives outside where the assets are supposed to live? Or is the idea that if you call readAsset on anything other than something generated at build time by Vite, then you deserve whatever you get?

@Rich-Harris
Copy link
Member Author

Should it be the responsibility of each adapter's readAsset function to check that it's not being asked to read some potentially sensitive file that lives outside where the assets are supposed to live?

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 statSync unnecessary by passing the content length (and content type) to the readAsset implementation...

@Conduitry
Copy link
Member

Oh, I like the idea of readAsset being passed the metadata which is already in-memory, rather than requiring it to hit the disk.

@Rich-Harris
Copy link
Member Author

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 readAsset implementation just so it can be returned in the form of response headers. Instead, readAsset has a simpler job, namely converting a filepath into a ReadableStream.

As a bonus, we're now able to throw an error if readAsset is called with something other than a Vite-generated asset path.

There's some room for improvement in the implementation but I feel good about this direction.

@eltigerchino eltigerchino added feature request New feature or request adapters - general Support for functionality general to all adapters and removed adapters - general Support for functionality general to all adapters labels Jan 17, 2024
}

if (manifest_data.hooks.server) {
// TODO if hooks.server.js imports `read`, it will be in the entry chunk
Copy link
Member

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?

Copy link
Member Author

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>;
Copy link
Member

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?

Copy link
Member Author

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

packages/kit/src/utils/features.js Outdated Show resolved Hide resolved
packages/kit/src/runtime/app/server/index.js Show resolved Hide resolved
packages/kit/src/exports/vite/index.js Outdated Show resolved Hide resolved
packages/kit/src/exports/vite/index.js Show resolved Hide resolved
@@ -289,7 +289,7 @@ importers:
dependencies:
'@sveltejs/kit':
specifier: ^1.0.0 || ^2.0.0
version: link:../kit
version: 2.3.5(@sveltejs/vite-plugin-svelte@3.0.1)(svelte@4.2.8)(vite@5.0.11)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm. this is weird

packages/kit/src/types/internal.d.ts Show resolved Hide resolved
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
@gitaarik
Copy link

This implementation has some limitations:

  • Hard to generate HTTP ETAG header for smart caching
  • No support for data: images that are not base64 encoded, like SVGs with just raw XML

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Server asset path mismatch after deployment Server asset path mismatch when running vite preview
7 participants