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

Test attempting to use server assets in production #8894

Closed
wants to merge 1 commit into from

Conversation

kevlened
Copy link
Contributor

If you want to reference a file buffer on the server, you're unable to, because the remix plugin will move the server assets to the client build. This seems intentional, but I'm not sure how to include static resources otherwise.

  • Tests - I'm unsure how you'd like to support binary blobs in the test fixture, but this PR has the bones

Testing Strategy:

I attempted to run the integration test, but the binary file couldn't be loaded.

I created a sample repo using the vanilla vite template (remix-run/remix/templates/vite) as a base: https://github.com/kevlened/remix-vanilla/pull/2/files

Copy link

changeset-bot bot commented Feb 26, 2024

⚠️ No Changeset found

Latest commit: 8d88c69

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

Copy link
Member

@markdalgleish markdalgleish left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed description and repro.

In your failing test case, the pdfFile value is meant to be a publicly addressable URL, not designed for use with fs.readFile, e.g. the URL could point to a CDN.

Instead, if you're wanting to get access to file contents while ensuring that the file is part of the module graph, you should use ?raw to import the asset as a string: https://vitejs.dev/guide/assets#importing-asset-as-string

I'm closing this PR, but feel free to re-open if I've missed something.

@kevlened
Copy link
Contributor Author

I use private binary files as templates on the server, so these can't be hosted assets.

When ?raw is used on a file that isn't utf-8 encoded (like a pdf), there doesn't seem to be a way to recover the original bytes. I'd also expect ?url to provide base64, but it doesn't.

I've tried other plugins such as vite-plugin-arraybuffer, but that causes other issues.

The most natural approach seems to be retaining the files from the ssrEmitsAssets flag. I'm happy to hack around this limitation at the moment by copying these assets from the client assets back to the server assets following the build, but there would ideally be a flag to override this behavior.

@kevlened
Copy link
Contributor Author

After moving the files back, I still get a missing file error, so I'll have to investigate a bit more.

@kevlened
Copy link
Contributor Author

While I was able to hack around this, the cleanest solution was to resolve my issues with the vite-plugin-arraybuffer by using an older version.

@markdalgleish
Copy link
Member

markdalgleish commented Feb 27, 2024

Nice one, looks like you managed to get vite-plugin-arraybuffer fixed in v0.0.5. To me this plugin is the right solution.

We could have fixed this by copying static assets from the server build rather than moving them, but what we've been trying to avoid is deploying a bunch of unused static assets as part of the server bundle. If your code references a file URL or a CSS file, conceptually that's part of the client bundle, even if it's only ever imported in the server graph (so that it can be passed down via a loader).

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

Successfully merging this pull request may close these issues.

2 participants