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

Vite Plugin migration path, with some findings #7889

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions contributors.yml
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,7 @@
- phishy
- plastic041
- plondon
- plopix
- pmbanugo
- prasook-jain
- pratikdevdas
Expand Down
60 changes: 48 additions & 12 deletions docs/future/vite.md
Original file line number Diff line number Diff line change
Expand Up @@ -90,23 +90,59 @@ Remix is now just a Vite plugin, so you'll need to hook it up to Vite.

```ts filename=vite.config.ts
import { unstable_vitePlugin as remix } from "@remix-run/dev";
import { defineConfig } from "vite";

export default defineConfig({
plugins: [remix()],
});
import tsconfigPaths from "vite-tsconfig-paths";
import { defineConfig, loadEnv } from "vite";

export default ({ mode }: { mode: string }) => {
const env = loadEnv(mode, process.cwd(), "");
Copy link
Member

Choose a reason for hiding this comment

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

This has been fixed in #7958 and will be out in the next release.

process.env = { ...process.env, ...env };
return defineConfig({
plugins: [
remix(),
tsconfigPaths(),
],
});
};
```

The subset of [supported Remix config options][supported-remix-config-options] should be passed directly to the plugin:

```ts filename=vite.config.ts lines=[3-5]
export default defineConfig({
plugins: [
remix({
ignoredRouteFiles: ["**/.*"],
}),
],
});
export default ({ mode }: { mode: string }) => {
const env = loadEnv(mode, process.cwd(), "");
process.env = { ...process.env, ...env };
return defineConfig({
plugins: [
remix({
ignoredRouteFiles: ["**/.*"],
}),
tsconfigPaths(),
],
});
};
```

#### CSS Import
Copy link
Member

Choose a reason for hiding this comment

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

This is already covered in the "Fix up CSS imports" section.


If you were importing file like this

```ts
import stylesheet from "~/styles/app.css";

export const links: LinksFunction = () => [
...(cssBundleHref ? [{ rel: "stylesheet", href: cssBundleHref }] : []),
{ rel: "stylesheet", href: stylesheet },
];
```

now you can just do this:
Copy link
Member

Choose a reason for hiding this comment

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

I strongly advise against recommending this. https://www.epicweb.dev/tips/do-not-bundle-all-your-css

Copy link
Author

Choose a reason for hiding this comment

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

I can remove that I was recommended to add it.... so you think that's better to add the ?inline ?

Copy link
Author

@Plopix Plopix Nov 7, 2023

Choose a reason for hiding this comment

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

declare module '*.css' {
  /**
   * @deprecated Use `import style from './style.css?inline'` instead.
   */
  const css: string
  export default css
}

Copy link
Member

Choose a reason for hiding this comment

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

I just mean it's better to actually import the stylesheet and include it itself in most cases.

Copy link
Author

Choose a reason for hiding this comment

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

I understand, it is just that without doing anything, developer will have to understand the change to be made.
Either do the thing you don't recommend or add the ?inline which by itself seemed weird to me too.
So we have to make a decision, or mention both in the migration path I guess.
Let me know!

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why any change needs to be made though. With vite can you not import the href anymore? That sounds wrong (and bad). It would probably be best to get an actual Remix maintainer involved to make a decision here...

Copy link
Author

Choose a reason for hiding this comment

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

of course that's just a sharing feedback here. I am no one to take a decision but yet, indeed it displays a big warning if you don't do anything:

Screenshot 2023-11-08 at 12 26 27 PM

Screenshot 2023-11-08 at 12 27 13 PM

So all I am saying is (and the only goal of that PR): helping developer in the migration paths :)

Which ever is the decision, I will update the PR

Thank @kentcdodds to spot that, it's indeed very important to point people in the good direction!

Copy link
Member

Choose a reason for hiding this comment

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

In theory Vite can support importing the href of a CSS file, but Vite currently has an issue where CSS files imported with the ?url query string don't go through the CSS transformation pipeline. This means tools like PostCSS/Tailwind won't apply to your CSS. We have an issue tracking it from a Remix perspective here: #7786.

In addition to this issue, it's also worth considering that we're trying to pick our battles a bit more when it comes to bundling so that we're better positioned to leverage tools like Vite. As a result, we're targeting CSS side-effect imports as the primary way of managing CSS since this lines up with Vite: https://vitejs.dev/guide/features.html#css.


```ts
import "~/styles/app.css";

export const links: LinksFunction = () => [
...(cssBundleHref ? [{ rel: "stylesheet", href: cssBundleHref }] : []),
];
```

#### TypeScript integration
Expand Down