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

Revert "feat: include server assets for Vercel serverless functions" #11644

Merged
merged 1 commit into from
Jan 16, 2024

Conversation

eltigerchino
Copy link
Member

@eltigerchino eltigerchino commented Jan 16, 2024

Reverts #10979 (in case we need to revert it to unblock a pending release)

Copy link

changeset-bot bot commented Jan 16, 2024

⚠️ No Changeset found

Latest commit: 608f58b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Rich-Harris
Copy link
Member

Yeah, I'm afraid I think we need to take another run at this. Merely copying the assets over probably isn't all that useful unless there's a way to actually read them on the server; for that I think we need something like serverAssets described in #11020 (comment). Will take a look at that

@Rich-Harris Rich-Harris merged commit 972a985 into main Jan 16, 2024
13 checks passed
@Rich-Harris Rich-Harris deleted the revert-10979-feat-server-assets branch January 16, 2024 14:17
@eltigerchino
Copy link
Member Author

eltigerchino commented Jan 16, 2024

Merely copying the assets over probably isn't all that useful unless there's a way to actually read them on the server

It currently works for the Vercel adapter because they get copied to the same path as the Vite resolved URL after building. Should I open a new PR with the changes made from the feedback in #10979 (comment) so the Vercel side can benefit from this in the mean time?

Netlify is a different story as we don't have to copy anything. All assets are already accessible but they live in .netlify/server when deployed. Node is similar, where the Vite resolved path is /_app/immutable/assets/* but our adapter copies the assets to server/_app/immutable/assets/* and the root relative path is inaccurate, so there's a mismatch.

@Rich-Harris
Copy link
Member

We need to answer the question 'how do I read from the filesystem in an idiomatic way?' and work backwards from there. Mucking around with things like .netlify/server is no good. I made a start on this in #11647 but I'm not 100% sure if it's the right path

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

Successfully merging this pull request may close these issues.

2 participants