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(lib)!: use package name for css output file name #18488

Merged
merged 9 commits into from
Oct 30, 2024
Merged

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Oct 28, 2024

Description

fix #4863

  • Use package name for css output file name
  • Can be customized if build.lib.fileName is changed to a string or function
    • If passed a function, the format argument can now be "css"

docs/guide/migration.md Outdated Show resolved Hide resolved
packages/vite/src/node/__tests__/plugins/css.spec.ts Outdated Show resolved Hide resolved
@sapphi-red
Copy link
Member

Thanks 💚

Co-authored-by: 翠 / green <green@sapphi.red>
@patak-dev
Copy link
Member

Thinking on the breaking changes, maybe we could try to avoid them? Maybe we'd want to move build.rollupOptions.xxxFileNames to build so we can have an easier upgrade later on from rolldown and it may be good if lib.fileName could be configured directly with these? I don't know if the breaking change is good if we may change it later, and mixing module format with 'css' feels a bit strange to me. Maybe a new experimental lib.cssFileName could be added for now?

@bluwy
Copy link
Member Author

bluwy commented Oct 28, 2024

I'm not sure if we should abstract build.rollupOptions.xxxFileNames, that's quite a large scope and it's unclear how it'll adapt with rolldown yet. lib.cssFileName could be interesting. I also find mixing it into fileName is quite breaking. I'm fine with adding cssFileName (I don't think it has to be experimental).

I assume we still want to break the default naming of style.css to my-lib-name.css though? That's one of the reasons we waited till the major to make the change. Or do you think cssFileName should default to "style"?

@patak-dev
Copy link
Member

patak-dev commented Oct 28, 2024

Agreed on not trying to move from rollupOptions yet. Do you see a future where we just keep saying people should configure these things through a new rolldownOptions? I was thinking we should try to avoid that at least for common options (you could argue that xxxFileNames is low level enough to not have it in vite though)

Or do you think cssFileName should default to "style"?

I was thinking on that. If we keep it as style, it would be because we don't think the breaking change will give users much benefit. Having a way to set the names themselves, it feels a bit unneeded.

@sapphi-red
Copy link
Member

I think xxxFileNames is low level enough not to have it outside rollupOptions. But I think the entry file name is something that should be easy to configure as that is something the users of the library would involve (for example, in a case where serving your built UMD/IIFE file on CDN, the library users need to specify the filename).

Having a way to set the names themselves, it feels a bit unneeded.

Isn't it the same with build.lib.fileName?

@patak-dev
Copy link
Member

With build.lib.fileName you couldn't change the css file name before this PR, I think I'm missing something about the question.

@sapphi-red
Copy link
Member

I thought that you think build.lib.cssFileName is a bit unneeded (correct me if I'm understanding wrongly). In that case, do you think build.lib.fileName is also unneeded? If you don't think build.lib.fileName is unneeded, where does the difference between build.lib.fileName and build.lib.cssFileName come from?

@patak-dev
Copy link
Member

I thought that you think build.lib.cssFileName is a bit unneeded (correct me if I'm understanding wrongly). In that case, do you think build.lib.fileName is also unneeded? If you don't think build.lib.fileName is unneeded, where does the difference between build.lib.fileName and build.lib.cssFileName come from?

What I meant was that the breaking change from swapping the default from style to the package name was a bit unneeded if you now have a way to change it.

@bluwy
Copy link
Member Author

bluwy commented Oct 28, 2024

Do you see a future where we just keep saying people should configure these things through a new rolldownOptions?

I think so. If we bring an option into Vite, I think it should also apply elsewhere outside of a rollup/rolldown bundle for it to be worth the maintenance. Otherwise we're guaranteeing it to work if we switch bundlers. Vite's structure had also been about relaying tooling responsibilities especially for advanced configurations 🤔


Personally I think it's better to break the name now that we have the chance. We want users to move to Vite 6 but not extra needed for lib mode users I think. Between re-using fileName or cssFileName, I guess the latter is more ergonomic if you have multiple formats for the JS file and you can assign a one-liner function to it. cssFileName also makes it easier for users to migrate if they're lazy, simply cssFileName: 'style'. The lack of function form could be nice too as this PR doens't handle multiple CSS files from different entries (via cssCodeSplit: true) yet. That one is harder to implement for me.

@sapphi-red
Copy link
Member

What I meant was that the breaking change from swapping the default from style to the package name was a bit unneeded if you now have a way to change it.

Ah, I see. I also think it's a good chance.

I quite like Blu's idea of telling users to add cssFileName: 'style'. That would make the migration more simple.

@patak-dev patak-dev merged commit 61cbf6f into main Oct 30, 2024
15 checks passed
@patak-dev patak-dev deleted the css-lib-file-name branch October 30, 2024 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Allow to specify filename of emitted CSS when build.cssCodeSplit: false
3 participants