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: align default chunk and asset file names with rollup #10927

Merged
merged 6 commits into from
Nov 17, 2022

Conversation

patak-dev
Copy link
Member

Description

Both rollup and esbuild use [name]-[hash].[extname] for their default chunk and asset file names. In Vite the default is [name].[hash].[extname] (for historical reasons according to Evan)

I think we should use the opportunity of Vite 4 to align with the closest tools in the ecosystem. @benmccann and @danielroe already said this would be fine with them. @matthewp @bluwy interested to know Astro's position. Given that assetFileNames and chunkFileNames are configurable, the ecosystem shouldn't be relying on our defaults, but there may be CI tests failures if there are regexes against the default like we had in Vite (cc @dominikg)


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@matthewp
Copy link
Contributor

In Astro we override these (even when they mirror Vite's current defaults) so no opinion on this change from our end. I'm not sure what aligning with other tools buy you. Perhaps hosts will use the convention to know if an asset can have an immutable cache header? Although I think some hosts already do this based on what framework outputs, cc @stramel who I think has implemented this in the past.

@patak-dev
Copy link
Member Author

@matthewp aligning would only make it a bit easier for people to move between rollup, esbuild, and Vite because the defaults are the same. In the sense of least surprise, docs, etc. I think there isn't a technical reason. @benmccann suggested a simplification in vite-plugin-pwa, but this was because the plugin had a regex against the default that has been already corrected by @userquin.

@patak-dev patak-dev added the p3-significant High priority enhancement (priority) label Nov 14, 2022
@patak-dev
Copy link
Member Author

@brc-dd looks like this PR would break VitePress, do you know if you are relying on the format of output files to be [name].[hash].[extname] somewhere?

@brc-dd
Copy link
Contributor

brc-dd commented Nov 14, 2022

Yeah, there is this line: https://github.com/vuejs/vitepress/blob/cc91d555b5bfcbde35f2ba33aedcd79a5cef713b/src/node/plugin.ts#L18

I'll explicitly add assetFileNames there for now.

@userquin
Copy link
Contributor

userquin commented Nov 14, 2022

Yeah, there is this line: https://github.com/vuejs/vitepress/blob/cc91d555b5bfcbde35f2ba33aedcd79a5cef713b/src/node/plugin.ts#L18

I'll explicitly add assetFileNames there for now.

You can just change it to support both formats: PWA plugin Will use this approach.

VitePress will need some changes if you want to align it with Vite:
https://github.com/vuejs/vitepress/blob/main/src/node/build/bundle.ts#L77

@patak-dev
Copy link
Member Author

Supporting both isn't a bad idea for the moment, but it would be better to check how to avoid depending on the format because the user can change it. IIUC, VitePress doesn't prevent that, no?

@brc-dd
Copy link
Contributor

brc-dd commented Nov 14, 2022

Supporting both isn't a bad idea for the moment

There are many places where we are relying on that format. For example, in client, the requests are made according to that format. Supporting both will likely get messy (as the client guesses the chunk name from hash).

IIUC, VitePress doesn't prevent that, no?

Never tested. Also, I don't know why anyone would do that. Moreover, if the vite config is added through .vitepress/config.ts, then we are simply overriding those values.

@brc-dd
Copy link
Contributor

brc-dd commented Nov 15, 2022

@patak-dev bump vp's version, it should work now.

@patak-dev
Copy link
Member Author

Thanks @brc-dd!

Copy link
Collaborator

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

thanks. I think this is nice. it's very minor for us especially since vite-plugin-pwa has now been updated as you mentioned, but I do think it's nice to keep things aligned with rollup

@patak-dev
Copy link
Member Author

We discussed this PR in today's team meeting and we think it is a good idea to move forward with it.

@patak-dev patak-dev merged commit cc2adb3 into main Nov 17, 2022
@patak-dev patak-dev deleted the feat/align-asset-and-chunks-naming-with-rollup branch November 17, 2022 10:35
fc pushed a commit to fc/vite that referenced this pull request Nov 23, 2022
rnathuji added a commit to openstax/k12-apps-raise that referenced this pull request Jan 13, 2023
Per vitejs/vite#10927, there was a change
made as part of vite 4 to align default build file names with the
default from rollup. While this is mostly superficial for us, since
the change causes a slight difference in the production filename
artifact I'm explicitly adding a configuration for it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-significant High priority enhancement (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants