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

fix(css): use non-nested chunk name if facadeModule is not CSS file #15155

Merged
merged 4 commits into from Nov 28, 2023

Conversation

sapphi-red
Copy link
Member

@sapphi-red sapphi-red commented Nov 27, 2023

Description

#14945 changed the CSS chunk names to respect chunk name correctly.
That fixed the following cases:

But this also changed the CSS chunk name when a nested JS entry chunk imports a CSS chunk. For example,

build: {
  rollupOptions: {
    input: {
      'nested/foo.js': 'nested/foo.js' // nested/foo.js imports a CSS file
    }
  }
}

In this case, some people would want the CSS file to be output under nested directory, while others would want the CSS file to be output without nested directory.
The current behavior is the latter one, and this PR changes to the former one. But I'm not sure which is better. 🤔

refs #14945
refs #15141

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines, especially the Pull Request Guidelines.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Update the corresponding documentation if needed.
  • Ideally, include relevant tests that fail without this PR but pass with it.

@sapphi-red sapphi-red added feat: css p3-minor-bug An edge case that only affects very specific usage (priority) labels Nov 27, 2023
@bluwy
Copy link
Member

bluwy commented Nov 27, 2023

The current behavior is the latter one, and this PR changes to the former one. But I'm not sure which is better. 🤔

Do you mean the current behaviour is the former, and this PR changes to the latter instead? If I'm reading the PR right.

I tested the linked issues and it seems to not regress them, in which case if the only change is

But this also changed the CSS chunk name when a nested JS entry chunk imports a CSS chunk.

Then I think we can fix this so it returns to the behaviour as before. I think we could fix this as a patch since this output shouldn't be explicitly relied upon before.

@patak-dev
Copy link
Member

I also think it is a good idea to get back to the previous behavior. I think it is less surprising that imported CSS works in the same way, no matter if they are imported from an entry or not (comment). So we keep the special handling only for entries.

It would be good if SvelteKit wouldn't depend on this though in the future.

@sapphi-red
Copy link
Member Author

Do you mean the current behaviour is the former, and this PR changes to the latter instead? If I'm reading the PR right.

Oops. Yes 😅
👍 I'll add tests to this PR tomorrow.

@benmccann
Copy link
Collaborator

It would be good if SvelteKit wouldn't depend on this though in the future.

I don't think we do, but it was the issue fixed in #15154 which was causing us trouble. Now that it's fixed hopefully we don't depend on this anymore.

@sapphi-red sapphi-red marked this pull request as ready for review November 28, 2023 13:17
@patak-dev
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 9bf031d: Open

suite result latest scheduled
analogjs success success
astro success success
histoire failure failure
ladle success success
laravel failure failure
marko success success
nuxt failure failure
nx success success
previewjs success success
qwik success success
rakkas success success
sveltekit failure failure
unocss success success
vike success success
vite-plugin-pwa success success
vite-plugin-react success success
vite-plugin-react-pages success success
vite-plugin-react-swc success success
vite-plugin-svelte success success
vite-plugin-vue success success
vite-setup-catalogue success success
vitepress success success
vitest success success

@patak-dev patak-dev merged commit 811e392 into vitejs:main Nov 28, 2023
10 checks passed
@sapphi-red sapphi-red deleted the fix/css-use-non-nested-chunk-name branch November 29, 2023 05:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: css p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants