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

Multiple CSS files have the same name when using the Vite plugin with a scoped package #631

Closed
AndrewLeedham opened this issue Mar 22, 2022 · 11 comments · Fixed by #647
Closed

Comments

@AndrewLeedham
Copy link
Contributor

Describe the bug

If the package name includes a scope e.g @vanilla-extract/example the outputted CSS files will all have the names example.vanilla.css, example.vanilla2.css, example.vanilla3.css etc. This seems to be because the sanitise file scope function adds the package name and when a scope is used it includes a / so it considers the last part as the file name. I would expect the file name to be included in the asset name, which it is when not using a scope.

Link to reproduction

https://stackblitz.com/edit/vitejs-vite-gfnuxw?file=vite.config.ts

  • Run npm run build in the above and it outputs:
    vite v2.8.4 building for production...
    ✓ 9 modules transformed.
    dist/main.js                0.25 KiB / gzip: 0.20 KiB
    dist/main.css.js            0.18 KiB / gzip: 0.16 KiB
    dist/Button.js              0.13 KiB / gzip: 0.13 KiB
    dist/Heading.js             0.14 KiB / gzip: 0.14 KiB
    dist/Button.css.js          0.18 KiB / gzip: 0.16 KiB
    dist/Heading.css.js         0.18 KiB / gzip: 0.16 KiB
    dist/example.vanilla.css    0.23 KiB / gzip: 0.16 KiB
    dist/example.vanilla2.css   0.12 KiB / gzip: 0.13 KiB
    dist/example.vanilla3.css   0.04 KiB / gzip: 0.06 KiB
    
  • However, if you remove the scope from package.json->name. It outputs the following:
    dist/main.js                                0.25 KiB / gzip: 0.20 KiB
    dist/main.css.js                            0.19 KiB / gzip: 0.15 KiB
    dist/Button.js                              0.13 KiB / gzip: 0.13 KiB
    dist/Heading.js                             0.14 KiB / gzip: 0.14 KiB
    dist/Button.css.js                          0.20 KiB / gzip: 0.16 KiB
    dist/Heading.css.js                         0.20 KiB / gzip: 0.16 KiB
    dist/Button.css.ts___example.vanilla.css    0.23 KiB / gzip: 0.17 KiB
    dist/main.css.ts___example.vanilla.css      0.12 KiB / gzip: 0.13 KiB
    dist/Heading.css.ts___example.vanilla.css   0.04 KiB / gzip: 0.06 KiB
    

System Info

Output of npx envinfo --system --npmPackages @vanilla-extract/css,@vanilla-extract/webpack-plugin,@vanilla-extract/esbuild-plugin,@vanilla-extract/vite-plugin,@vanilla-extract/sprinkles,webpack,esbuild,vite --binaries --browsers:

  System:
    OS: macOS 11.2.3
    CPU: (12) x64 Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
    Memory: 35.36 MB / 16.00 GB
    Shell: 5.8 - /usr/local/bin/zsh
  Binaries:
    Node: 16.13.2 - ~/.nvm/versions/node/v16.13.2/bin/node
    Yarn: 1.22.17 - /usr/local/bin/yarn
    npm: 8.1.2 - ~/.nvm/versions/node/v16.13.2/bin/npm
  Browsers:
    Chrome: 99.0.4844.83
    Edge: 99.0.1150.46
    Firefox: 94.0.1
    Safari: 14.0.3
  npmPackages:
    @vanilla-extract/esbuild-plugin: ^2.0.3 => 2.0.3 
    @vanilla-extract/vite-plugin: ^3.1.2 => 3.1.2 
    esbuild: 0.14.22 => 0.14.22 
    vite: 2.8.4 => 2.8.4

Related

@AndrewLeedham
Copy link
Contributor Author

@graup pointed out in a conversation we had on discord that "virtual" modules should not be necessary for this case:

but the Vite documentation says that "virtual" should not be necessary for cases where the output is derived from a real file
https://vitejs.dev/guide/api-plugin.html#virtual-modules-convention

So I had a go at removing them, this is very rough around the edges and may re-introduce past bugs (e.g the .. thing) and likely break other stuff. But for the issue at hand it fixes paths: master...AndrewLeedham:AL-proper-paths.

This seems like the right direction to me, as Vite will see the CSS file properly, and it will have a proper file system path for generating the name. Curious on your thoughts here @mattcompiles?

@mattcompiles
Copy link
Contributor

I have thought about this in the past as well. VE CSS is derived from a real file, however it is not derived from an AST transform or compilation process. Meaning it can't be sensibly mapped with a source map. Therefore I believe they do need to represented as virtual files. However feel free to correct me if I'm wrong and there is a way to do this I'm not aware of.

Either way though I think issue can be resolved even with virtual files. We might just need to update the file scope serialization.

@AndrewLeedham
Copy link
Contributor Author

I see why sourcemaps wouldn't be possible, but is there any benefit of using virtual files over an "actual" path without sourcemaps?

@AndrewLeedham
Copy link
Contributor Author

Reading through both the Vite and Rollup docs some more I guess it prevents other plugins from processing the import. Although that seems to be a feature of \0 which isn't in use here. Also it would seem we do want other plugins to process the import e.g vite:css.

A few potential options:

  1. Replace virtual:vanilla-extract:style.vanilla.css$$$example-package with ./style.css, and just don't provide sourcemaps.
  2. Refactor virtual:vanilla-extract:style.vanilla.css$$$example-package to something like virtual:vanilla-extract:style.css?package=example-package.
  3. Provide some-sort of plugin option that allows you to format the last part of the path given the filescope. E.g:
    fileNames: (fileScope) => fileScope.filePath which would be used to create something like virtual:vanilla-extract:<user-value>?package=example-package.
  4. Something else?

Curious on your thoughts here @mattcompiles. Happy to take a swing at implementing an approach, just not sure which direction to go.

@mattcompiles
Copy link
Contributor

mattcompiles commented Mar 25, 2022

Would this issue be resolved if we just make the file path be the last thing in the filescope string instead? I don't remember any specific reason it was last.

e.g.

export function stringifyFileScope({
  packageName,
  filePath,
}: FileScope): string {
  return packageName ? `${packageName}$$$${filePath}` : filePath;
}

If so, happy to accept a PR. 😄

@graup
Copy link
Collaborator

graup commented Mar 25, 2022

Would this issue be resolved if we just make the file path be the last thing in the filescope string instead? I don't remember any specific reason it was last.

e.g.

export function stringifyFileScope({
  packageName,
  filePath,
}: FileScope): string {
  return packageName ? `${packageName}$$$${filePath}` : filePath;
}

Yes I believe that would be a slight improvement and fix this particular issue.

@AndrewLeedham
Copy link
Contributor Author

That would definitely fix this issue yes. It still doesn't feel like the right solution to me though, because we are relying on the fact that Vite uses chunk.name which in this case will be the basename of the virtual ID. But probably better to get a solution in for now as it shouldn't affect anything else.

@graup
Copy link
Collaborator

graup commented Mar 30, 2022

Referring back to @AndrewLeedham 's comment, how about something like this? (Untested poc: remove prefix and change serialization to filePath?package=packageName)

As far as I can tell the packageName is only there for the hot reload and injectStyles, right?

@mattcompiles
Copy link
Contributor

The package name is in the filescope as the filename is relative to that package. If you're using a package that contains VE files then all the file paths coming from that package will be relative to their package.json. What we need to be careful of here is allowing two separate files from having the same ID. This solution is probably ok from that regard.

One thing I'm not clear on though is how this solution is more ideal than what I suggested? Could someone just explain why this creates a better build output?

@graup
Copy link
Collaborator

graup commented Mar 31, 2022

The main advantage is that having the generated css files as proper (non-virtual) modules, with usable names and paths, works better with preserveModules and also possibly other plugins/options. Seems more flexible to me. (Didn't check but it might also enable us to remove the postcss code from the plugin and just have css files be processed by the rest of the rollup plugin chain.)

@mattcompiles
Copy link
Contributor

This issue, along with a few others have lead me to doing a decent refactor of file scopes across the board. Hoping this PR will make everyone happy. #647

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants