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

fix(vite): remove server exports from resource routes #7828

Merged

Conversation

pcattori
Copy link
Contributor

@pcattori pcattori commented Oct 30, 2023

Fixes #7795

Testing Strategy

Added integration cases to vite build test checking that server values get removed from client, and that build succeeds without errors referring to server code.

Test was failing before the change, and passes now

@changeset-bot
Copy link

changeset-bot bot commented Oct 30, 2023

🦋 Changeset detected

Latest commit: acc17a2

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

This PR includes changesets to release 16 packages
Name Type
@remix-run/dev Patch
create-remix Patch
remix Patch
@remix-run/architect Patch
@remix-run/cloudflare Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/css-bundle Patch
@remix-run/deno Patch
@remix-run/eslint-config Patch
@remix-run/express Patch
@remix-run/node Patch
@remix-run/react Patch
@remix-run/serve Patch
@remix-run/server-runtime Patch
@remix-run/testing Patch

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

@pcattori pcattori force-pushed the pedro/fix-vite-resource-route-server-code-removal branch from 8f2788b to 68ef921 Compare October 30, 2023 21:46

// grep for server-only values in client assets
let result = shell
.grep("-l", /SERVER_ONLY_1|SERVER_ONLY_2/, assetFiles)
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

Copy link
Member

Choose a reason for hiding this comment

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

Should this be simplified to target just "SERVER_ONLY" for both exports rather than numbering them? Makes modifying this test less error prone.

@@ -632,20 +632,6 @@ export const remixVitePlugin: RemixVitePlugin = (options = {}) => {

let serverExports = ["loader", "action", "headers"];

let routeExports = await getRouteModuleExports(
Copy link
Member

Choose a reason for hiding this comment

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

I like that the route transform doesn't need to run this anymore.

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.

Some minor feedback, nothing blocking. Looks great 👍

@pcattori pcattori merged commit fd0eb83 into release-next Oct 31, 2023
9 checks passed
@pcattori pcattori deleted the pedro/fix-vite-resource-route-server-code-removal branch October 31, 2023 01:55
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 2.2.0-pre.3 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Copy link
Contributor

🤖 Hello there,

We just published version 2.2.0-pre.4 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Copy link
Contributor

🤖 Hello there,

We just published version 2.2.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

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