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

Config option to opt out of CSS Injection #9465

Closed
4 tasks done
wessberg opened this issue Jul 31, 2022 · 6 comments
Closed
4 tasks done

Config option to opt out of CSS Injection #9465

wessberg opened this issue Jul 31, 2022 · 6 comments

Comments

@wessberg
Copy link

wessberg commented Jul 31, 2022

Description

It is currently possible to opt out of CSS Injection by adding ?inline to the relevant import declarations.

However, for some applications, specifically those that rely on Constructable Stylesheets or more generally applications that rely on Shadow DOM, adding this to the end of every import feels unnecessarily redundant to me.

Suggested solution

It would be nice if a configuration option could be exposed to just never inject any CSS. There is already a few CSS-specific configuration options, and I propose adding an additional option css.inject which defaults to true

Alternative

It can be worked around by adding ?inline to every import declaration referencing a CSS file.

Additional context

No response

Validations

@bluwy
Copy link
Member

bluwy commented Dec 30, 2022

I think this could be implemented as a Vite plugin at the meantime. We prefer to avoid adding another option unless it's not possible to be implemented as a plugin. Furthermore, it seems the feature request is only as a convenience to omit ?inline. This would then increase the surface area to support ?inject if someone wants the contrary. Closing this for now.

@bluwy bluwy closed this as not planned Won't fix, can't repro, duplicate, stale Dec 30, 2022
@wessberg
Copy link
Author

We prefer to avoid adding another option unless it's not possible to be implemented as a plugin

I know this is your stance, and I understand that for every non-configurable behavior, you have more flexibility internally to change things freely. However, as you've already allowed for certain CSS-specific configuration options, and since the default behavior of Vite here is very opiniated, I think this seems like something that would make a lot of sense to expose an option to disable

Furthermore, it seems the feature request is only as a convenience to omit ?inline

Adding ?inline to each and every CSS import in a large and evolving code base is a pretty big consideration in terms of adopting Vite in an organization, I'd argue, and I assume a plugin-based solution will slow down esbuild, as it tends to do, as it will have to check every file for relevant import statements and potentially perform a text replacement with something like MagicString and return a new SourceMap.

Having the default behavior being one in which there is a non-obvious side-effect feels unnatural to me, personally, as I'd argue the more natural behavior for importing named bindings from modules is one in which there are no side-effects. I realize there is precedent for something like this across some bundlers, but when browsers eventually support importing stylesheets via the ESM loading mechanism, I think its probably safe to assume it will be free of side-effects and return a CSSStyleSheet that can be adopted by a root programmatically, which Constructable StyleSheets laid the groundwork for.

But this is of course just my opinion. Happy New Year!

@bluwy
Copy link
Member

bluwy commented Jan 2, 2023

Happy new year! 🙂

since the default behavior of Vite here is very opiniated, I think this seems like something that would make a lot of sense to expose an option to disable

Vite is very strict with adding a new option too, so the rule has been if a feature can't be implemented as a plugin, then it should be an option. It's usually not added as a way to opt-out of it's opinionated nature.

and I assume a plugin-based solution will slow down esbuild, as it tends to do, as it will have to check every file for relevant import statements and potentially perform a text replacement with something like MagicString and return a new SourceMap.

I don't think you need to do that. You should be able to do something like:

const esbuildCssInline = {
  name: 'css-inline',
  setup(build) {
    build.onResolve({ filter: /\.css$/ }, ({ path }) => ({
      path: path + '?inline',
      external: true,
    }))
  }
}

Having the default behavior being one in which there is a non-obvious side-effect feels unnatural to me, personally, as I'd argue the more natural behavior for importing named bindings from modules is one in which there are no side-effects.

Vite 4 has deprecated default imports from .css files. So you can only do import './foo.css', so the intent is clear that it will have side effects. If you need to access the raw CSS value, you can use ?inline and default imports.

@github-actions github-actions bot locked and limited conversation to collaborators Jan 17, 2023
@vitejs vitejs unlocked this conversation Feb 6, 2023
@bluwy
Copy link
Member

bluwy commented Feb 6, 2023

(Temporarily unlocked so further discussions are public from a DM)

Re where to put the esbuild plugin, that should be put at optimizeDeps.esbuildOptions.plugins so during prebundling it handles the imports in dependencies. I think that's only part of the story though as you need to handle for the source code too.

So you might need this Vite plugin too:

const plugin = {
  name: 'css-inline',
  enforce: 'pre',
  resolveId(id, importer) {
    if (id.endsWith('.css') { // or a more robust check if needed
      return this.resolve(id + '?inline', importer)
    }
  }
}

I'm not sure if the esbuild plugin is needed with this Vite plugin (not sure why I recommended esbuild too in the first place). Maybe some tinkering could get what you want.

@wessberg
Copy link
Author

wessberg commented Feb 6, 2023

Thanks for your examples. You've made your stance clear, and I understand it. I do hope you will eventually expose this as a simple toggle to avoid asking users to implement one or more plugins to rewire the import mechanism for something that I'm confident will only continue to become the more dominant use case, considering the most likely standardization path.

If a config option isn't on the table, a little explainer or example of how to achieve this behavior without manually adding the ?inline param would be nice to see in the docs. 😊

@bluwy
Copy link
Member

bluwy commented Feb 11, 2023

I'm still not really sure this would gain traction because this breaks CSS from deps by default. I suppose when/if Vite supports constructable stylesheets directly it would make more sense.

@github-actions github-actions bot locked and limited conversation to collaborators Feb 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants