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

feat(css): build assets with the entry name when it is an entry point #11578

Merged
merged 4 commits into from
Aug 19, 2023

Conversation

aleen42
Copy link
Contributor

@aleen42 aleen42 commented Jan 4, 2023

REF: #4863 (comment)

How can we use the entry name to construct two files with the name L.css and X.css when specifying an entry like this:

export default {
    build : {
        rollupOptions : {
            input : {
                L : 'login/index.less',
                X : 'X/index.less'
            }
        }
    }
};

Inside the assetFileNames hook, we cannot distinguish two assets with the same name index.css.

In comparison with Webpack, it will use the entry name as the asset name when using the plugin, mini-css-extract-plugin.

Description

To resolve the problem mentioned above, I hope that Vite can use the entry name as the asset name.

fix #9877
close #9485


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.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • 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).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@aleen42 aleen42 changed the title breaking: build assets with the entry name when it is an entry point chore: build assets with the entry name when it is an entry point Jan 4, 2023
@aleen42 aleen42 force-pushed the entry-asset branch 3 times, most recently from 2b21f7d to 79777fc Compare January 6, 2023 05:41
@bluwy bluwy added feat: css p2-nice-to-have Not breaking anything but nice to have (priority) labels Apr 1, 2023
@bluwy bluwy changed the title chore: build assets with the entry name when it is an entry point feat(css): build assets with the entry name when it is an entry point Apr 1, 2023
@Maxim-Mazurok
Copy link

We need this feature as we have multiple pages that act as independent apps. Perhaps @patak-dev could have a review/merge this please?

const lang = path.extname(cssAssetName).slice(1)
const cssFileName = ensureFileExt(cssAssetName, '.css')
const cssFileName = ensureFileExt(
isEntry ? chunk.name : cssAssetName,
Copy link
Member

Choose a reason for hiding this comment

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

I think we should have this condition directly in line 552.

@patak-dev patak-dev added this to the 5.0 milestone May 15, 2023
@patak-dev
Copy link
Member

This looks good to me, but modifying the naming could be a breaking change. I added it to the Vite 5 milestone (to be released in September). @Maxim-Mazurok, in the meantime, I recommend you maintain a patch for Vite in your repo so you aren't blocked by Vite's release process.

@Maxim-Mazurok
Copy link

Thank you, we've decided to have two separate vite configs for now that extend a common one, seems to work fine for us for now

@sapphi-red
Copy link
Member

IIUC this will fix #9877 and close #9485.

@@ -37,7 +37,6 @@ describe.runIf(isBuild)('build', () => {
const cssAssetEntry = manifest['global.css']
const scssAssetEntry = manifest['nested/blue.scss']
const imgAssetEntry = manifest['../images/logo.png']
const dirFooAssetEntry = manifest['../../dir/foo.css'] // '\\' should not be used even on windows
Copy link
Member

Choose a reason for hiding this comment

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

Do you have a reason to remove this test? I think we shouldn't remove this one.

Copy link
Contributor Author

@aleen42 aleen42 Jun 7, 2023

Choose a reason for hiding this comment

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

Because the behaviour has broken and used the chunk name as the name of the assets, so should test manifest['bar.css'] and manifest['foo.css']. The case has not actually been removed.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't that because you changed this line?
https://github.com/vitejs/vite/pull/11578/files#diff-4fdf9a690fa55c7c730e03a900f729ce1afb886d64b4280cd4ce0f32d5844074L22-R22
I guess it would work if you revert that change.
I think you can append another entry point for the new test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if I revert the change, the chunk name has been used as the CSS asset name, instead of the path before, because this is an entry.

Copy link
Contributor Author

@aleen42 aleen42 left a comment

Choose a reason for hiding this comment

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

@sapphi-red Due to these changes, the asset name has used chunk.name instead of the path, which has broken the test case.

@sapphi-red
Copy link
Member

I restored the test for #9353 by adding a dynamic import chunk. Also I found that the Windows test was failing and fixed that as well.

@sapphi-red
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Aug 19, 2023

📝 Ran ecosystem CI: Open

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

@sapphi-red
Copy link
Member

histoire: faling with using yarn in packageManager field with pnpm (not related to this PR)
unocss: failing with type error not related to this PR
vite-plugin-react-pages: seems to be failing with HMR tests but given that this PR only changes renderChunk hook and that hook doesn't run in dev, I guess it's not related
vitepress: failing because of the change in #13646 (this PR's branch is a bit old)

@sapphi-red sapphi-red merged commit fd9a2cc into vitejs:main Aug 19, 2023
@lubomirblazekcz
Copy link
Contributor

@patak-dev @sapphi-red @aleen42

This actually led to bad entry being added into manifest.json, say you have a entry point src/styles/main.css, in manifest it will be main.css, which is wrong. Vite v4.5 has src/styles/main.css, which is correct and excepted.

Should I open new issue to this? I've noticed this is happening when I was testing v5 beta.

@patak-dev
Copy link
Member

@lubomirblazekcz could you create an issue with a minimal reproduction so we can better evaluate if this is expected or not?

@lubomirblazekcz
Copy link
Contributor

@patak-dev sure, here - #14943

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: css p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSS file name changed after v3 upgrade
7 participants