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(vite): strict route exports #8171

Merged
merged 2 commits into from
Dec 1, 2023
Merged

feat(vite): strict route exports #8171

merged 2 commits into from
Dec 1, 2023

Conversation

pcattori
Copy link
Contributor

@pcattori pcattori commented Nov 29, 2023

From the added docs:

With Vite, Remix gets stricter about which exports are allowed from your route modules.

Previously, Remix allowed user-defined exports from routes.
The Remix compiler would then rely on treeshaking to remove any code only intended for use on the server from the client bundle.

In contrast, Vite processes each module in isolation during development, so cross-module treeshaking is not possible.
You should already be separating server-only code into .server files or directories, so treeshaking isn't needed in those cases.
But routes are a special case since they intentionally blend client and server code.
Remix knows that exports like loader, action, headers, etc. are server-only, so it can safely remove them from the client bundle.
But there's no way to know when looking at a single route module in isolation whether user-defined exports are server-only.
That's why Remix's Vite plugin is stricter about which exports are allowed from your route modules.

In fact, we'd rather not rely on treeshaking for correctness at all.
Treeshaking is designed as a pure optimization, so relying on it for correctness is brittle.
Not only that, but relying on treeshaking also means that if tomorrow you or your coworker accidentally imports something you thought was client safe, you might end up with server code in your client bundle.
In short, Vite made us eat our veggies, but turns out they were delicious!

So instead of treeshaking, its better to be explicit about what code is server only.
For route modules, that means only exporting Remix route exports.
For anything else, put it in a .server file or directory instead.

As a simple mental model, think of a Remix route module like a function and the exports like named arguments to the function.

// Not real API, just a mental model
let route = createRoute({ loader, mySuperCoolThing });
//                                ^^^^^^^^^^^^^^^^
// Object literal may only specify known properties, and 'mySuperCoolThing' does not exist in type

Just like how you shouldn't pass unexpected named arguments to a function, you shouldn't create unexpected exports from a route module.
The result is that Remix is simpler and more predictable.


Additionally, user-defined exports are a footgun for HMR as the Remix and React Fast Refresh may not have a way to handle updates for custom exports. See https://remix.run/docs/en/main/discussion/hot-module-replacement#supported-exports .

This also results in a nice invariant:

Code in a Remix route only runs on for that route

👆 that may seem like just fancy words, but reliable invariants like that are invaluable to me when working on complex projects, esp. on larger teams and with Remix beginners.

Fullstack components?

@kentcdodds wrote a fantastic blog post about fullstack components in Remix. The idea is to colocate a component that depends on API with the route that provides that API. Specifically, Kent recommends colocating the component and loader in the same file.

Notably, the component uses useFetcher and manually fetches data for the API route it depends on rather than using useLoaderData as the whole point of a fullstack component is to use it anywhere, not just within routes nested under the route defining the loader. Indeed, "nesting" for resource routes isn't meaningful. As a result, the component depends on the API via that API's endpoint.

For example, with the default file routing convention in v2, you can still get colocation of this code in the same directory:

// app/routes/api/blah/route.ts

export const loader = () => {}
export type Loader = typeof loader
// app/routes/api/blah/component.ts
import type { Loader } from "./route"

export const FullstackComponent1 = () => {
  let fetcher = useFetcher<Loader>()
  // ... more code here ...
  fetcher.submit(
      { },
      { method: 'get', action: '/api/blah' },
    )
}

The only import needed from app/routes/api/blah/route.ts is the type for the loader. Notably, user-defined type exports are fine since they are stripped before the code runs.

Reasonable people can disagree, but I think this actually models the relationship between the route and the component more accurately. Just like any other part of my Remix code, if I need to access a loader from another route, I can fetch that endpoint and grab types from that route's loader by importing them.

Copy link

changeset-bot bot commented Nov 29, 2023

🦋 Changeset detected

Latest commit: db780a2

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 Minor
create-remix Minor
remix Minor
@remix-run/architect Minor
@remix-run/cloudflare Minor
@remix-run/cloudflare-pages Minor
@remix-run/cloudflare-workers Minor
@remix-run/css-bundle Minor
@remix-run/deno Minor
@remix-run/eslint-config Minor
@remix-run/express Minor
@remix-run/node Minor
@remix-run/react Minor
@remix-run/serve Minor
@remix-run/server-runtime Minor
@remix-run/testing 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

Copy link
Member

@mjackson mjackson left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for being so thorough here!

docs/future/vite.md Outdated Show resolved Hide resolved
docs/future/vite.md Outdated Show resolved Hide resolved
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.

Nice one! I've added a couple of docs suggestions, but this looks great.

@pcattori pcattori force-pushed the pedro/remix-only-exports branch 4 times, most recently from 1ab4e91 to 950d2c6 Compare December 1, 2023 06:49
@pcattori pcattori force-pushed the pedro/remix-only-exports branch from 950d2c6 to db780a2 Compare December 1, 2023 06:51
@pcattori pcattori merged commit 5120f82 into dev Dec 1, 2023
9 checks passed
@pcattori pcattori deleted the pedro/remix-only-exports branch December 1, 2023 07:05
@github-actions github-actions bot added the awaiting release This issue has been fixed and will be released soon label Dec 1, 2023
Copy link
Contributor

github-actions bot commented Dec 5, 2023

🤖 Hello there,

We just published version 2.4.0-pre.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!

Copy link
Contributor

🤖 Hello there,

We just published version 2.4.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!

@github-actions github-actions bot removed the awaiting release This issue has been fixed and will be released soon label Dec 13, 2023
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.

4 participants