Vite Plugin migration path, with some findings #7889
Conversation
|
|
Hi @Plopix, Welcome, and thank you for contributing to Remix! Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once. You may review the CLA and sign it by adding your name to contributors.yml. Once the CLA is signed, the If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run. Thanks! - The Remix team |
|
Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳 |
| ]; | ||
| ``` | ||
|
|
||
| now you can just do this: |
There was a problem hiding this comment.
I strongly advise against recommending this. https://www.epicweb.dev/tips/do-not-bundle-all-your-css
There was a problem hiding this comment.
I can remove that I was recommended to add it.... so you think that's better to add the ?inline ?
There was a problem hiding this comment.
declare module '*.css' {
/**
* @deprecated Use `import style from './style.css?inline'` instead.
*/
const css: string
export default css
}
There was a problem hiding this comment.
I just mean it's better to actually import the stylesheet and include it itself in most cases.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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:
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!
There was a problem hiding this comment.
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.
| }; | ||
| ``` | ||
|
|
||
| #### CSS Import |
There was a problem hiding this comment.
This is already covered in the "Fix up CSS imports" section.
| import { defineConfig, loadEnv } from "vite"; | ||
|
|
||
| export default ({ mode }: { mode: string }) => { | ||
| const env = loadEnv(mode, process.cwd(), ""); |
There was a problem hiding this comment.
This has been fixed in #7958 and will be out in the next release.
|
Thanks for the feedback and the PR! The issues you've raised have all been addressed so this update shouldn't be necessary anymore. If you'd like to confirm, you can try the latest nightly release. If there are any problems, feel free to open a separate issue. |
Just a little more doc on the migration path.
The ENV vars issue was a struggle (especially if you don't know Vite)
You may also want to look here: https://github.com/withastro/astro/tree/main/packages/astro/src/vite-plugin-env
Also, but that is probably a bug, I had to
npx remix revealotherwise I had this error:....node_modules...dev/dist/config/defaults/entry.server.node.tsx" outside of root directoryMost likely because I have PNPM workspaces and the
node_moduleis indeed outside. => but that should workI hope it helps