-
-
Notifications
You must be signed in to change notification settings - Fork 6k
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(dev): added assets to manifest #6649
Conversation
I would need some help solving the second part of issue #6595 if entry file is png, jpg, svg, etc. then the chunks are generated as empty This doesn't happend with
|
It'd be nice if you could add a test for this feature: https://github.com/lubomirblazekcz/vite/blob/manifest-assets/packages/playground/backend-integration/__tests__/backend-integration.spec.ts#L27 If the second part is a blocker, maybe you can modify the PR to support only CSS for now, and then PR again with a fix for other assets? Just to get things moving in that direction. In multiple back-end adapters, we had to add a plugin that transforms the manifest to support finding CSS files, so this would be really nice to have. |
@@ -101,10 +101,18 @@ export function manifestPlugin(config: ResolvedConfig): Plugin { | |||
return manifestChunk | |||
} | |||
|
|||
function createAsset(chunk: OutputAsset): ManifestChunk { | |||
return { | |||
file: chunk.fileName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having src
here if it exists would be helpful too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't be the src same as the name of the asset? Also not sure how to do this, asset returns only name
if (chunk.name) {
manifestAsset.src = chunk.name
}
Would this be correct? I don't know how to get full path for assets, they don't include facadeModuleId
Hi @innocenzi I've added the tests here https://github.com/vitejs/vite/pull/6649/files#diff-f21fb4f1a20cc108f9e9d6c1f086b7d74bf973bf2646e73d2087a8ec28a1e5ad The double css is there because of the vite config, could be resolved with something like this (line 19). Not sure if it's the best way or necessary though. .map((filename) => [path.relative(root, filename).replace('.css', ''), filename]) Also the second part is not blocker, it's still beneficial to have all assets in manifest. But it would be great if static assets could be added as entrypoint, but as I said - not sure how to fix that. |
# Conflicts: # packages/playground/backend-integration/__tests__/backend-integration.spec.ts
@innocenzi @patak-dev any chance getting this into v3? I think this would also solve #2375 and #4198 |
Would love this PR to be merged, but I think it's not completely done yet?
|
That is the current behaviour in vite now and it's not something this PR resolves. I will split the issue, so it's more clear. This PR resolves that CSS files and other (non entry assets) are generated into manifest as separate entries. |
@innocenzi I've changed the description of my issue and this PR, so it's clear what it resolves 👍 Also created a new issue regarding the problem I‘ve described in that previous comment - #8373 |
Thanks for this PR! I'm concerned about Given the following entry points: build: {
rollupOptions: {
input: [
'resources/js/app.js',
'resources/css/app.css',
],
},
} The manifest would end up as: {
"resources/js/app.js": {
"file": "assets/app.21787142.js",
"src": "resources/js/app.js",
"isEntry": true
},
"app.css": {
"file": "assets/app.2c05110b.css",
"src": "app.css"
}
} Without the full path, it's not really possible to correctly map from the source file to the compiled asset (unless you can trust there is only one file with that name). It's also inconsistent with the JS chunks and allows for naming collisions. I need to spend a bit more time digging through Rollup, but I'm worried this can't be resolved without an upstream change. |
@jessarcher in my tests For reference, this is from my tests in rollupOptions: {
input: [
'frontend/entrypoints/main.ts',
'frontend/entrypoints/global.css'
]
} {
"../images/logo.png": {
"file": "assets/logo.b9f46fb3.png",
"src": "../images/logo.png"
},
"main.ts": {
"file": "assets/main.ad17e0e9.js",
"src": "main.ts",
"isEntry": true
},
"global.css": {
"file": "assets/global.36c3a1e4.css",
"src": "global.css"
}
} |
@lubomirblazekcz I think that's because in the test, If the root was not a subdirectory, then This is because for the "chunk" type, it uses In In userland, the only way I can find to get the path is by grabbing it in the In Vite itself, I'm exploring a few options, such as keeping the original CSS chunk if it's an entrypoint, but I'm not sure on the impacts. The tests are a little funny because |
I've tried making the following change in - let isPureCssChunk = true
+ let isPureCssChunk = !chunk.isEntry This prevents the CSS chunk from being removed from the bundle so it can be added to the manifest, however, it doesn't have the desired result (via Before: {
'index.html': {
file: 'assets/index.html.6a442648.js',
src: 'index.html',
isEntry: true,
imports: [ 'main.ts' ],
css: [ 'assets/global.css.d99fc02a.css' ],
assets: [ 'assets/logo.b9f46fb3.png' ]
},
// ...
} After: {
'global.css': {
file: 'assets/global.css.9f516e50.js',
src: 'global.css',
isEntry: true,
css: [ 'assets/global.css.d99fc02a.css' ],
assets: [ 'assets/logo.b9f46fb3.png' ]
},
'index.html': {
file: 'assets/index.html.6a442648.js',
src: 'index.html',
isEntry: true,
imports: [ 'main.ts', 'global.css' ],
assets: [ 'assets/logo.b9f46fb3.png' ]
},
// ...
} The CSS entry point gets correctly added to the manifest (and will have a path where appropriate) however the When the CSS chunk is removed from the bundle, there doesn't seem to be any way to get its path correctly. I did notice that removed CSS chunks are stored in |
@jessarcher I see now, thanks for clearing it out. Seems this problem is only for css files. I will try to look into this. If you found suitable solution yourself I can add you to my forked project so you could add it in this PR. |
@lubomirblazekcz Oh, I didn't notice it was only a problem with CSS! After looking at const fileHandle = this.emitFile({
- name: cssAssetName,
+ name: chunk.facadeModuleId
+ ? normalizePath(path.relative(config.root, chunk.facadeModuleId))
+ : cssAssetName,
type: 'asset',
source: chunkCSS
}) I have no idea if it would be accepted, but feel free to add it or give me access 🙂 |
@jessarcher thanks that indeed fixed the issue! But it broke a bunch of tests, I will have to investigate further what went wrong :/ |
It broke @jessarcher I've invited you to the repo, feel free to add any changes. I will have more time at weekend to look at this more. |
b76d7e5
to
3147b88
Compare
const output = config.build?.rollupOptions?.output | ||
const assetFileNames = | ||
(output && !Array.isArray(output) | ||
? output.assetFileNames | ||
: undefined) ?? | ||
// defaults to '<assetsDir>/[name].[hash][extname]' | ||
// slightly different from rollup's one ('assets/[name]-[hash][extname]') | ||
path.posix.join(config.build.assetsDir, '[name].[hash][extname]') | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is copied verbatim from asset.ts
so we could potentially extract it to function if we wanted to keep that logic and naming pattern centralised.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, there was also a fix for plugin legacy with custom asset file names. So this abstraction was needed for it
@jessarcher great work! Everything seems to work good as expected now 👍 |
Nice! Thanks for your work here. We'll discuss this PR in the next team meeting. We need to also check in the meeting if the proposal to add a version field to the manifests needs to be done as part of these changes. See #7960 (comment). I would also appreciate your input regarding it to see how this will affect future updates to the manifest and the ability of integrations to cope with them. |
@patak-dev schema for manifests sounds like a great idea, it's something that for sure can be handy in the future |
This was a major blocker for Slinkity and Astro as we explore a special |
We talked about the PR in the last team meeting and we're good to move forward with it. We also discussed about adding a version and we think it isn't worth completely changing the current schema to include it. We'll merge it before cutting v3 beta next Monday. Let me know if there is something more needed here. @ElMassimo pinging you as this may also be interesting for Vite Ruby. |
This is fantastic news, thanks @patak-dev! |
Currently this change causes the entries to be generated incorrectly in Vite Ruby for any Sass/Less/Stylus stylesheet, as it's erasing the original extension from the manifest: "blue.css": {
"file": "assets/blue.75487b74.css",
"src": "blue.css"
} In contrasst, Vite does preserve "application.ts": {
"file": "assets/application.48a9de7f.js",
"src": "application.ts"
} For any backend integration to map files correctly, it's desirable to preserve the extension: "blue.scss": {
"file": "assets/blue.12fc1eef.css",
"src": "blue.scss"
} Are there any follow-up pull requests to this? |
Not yet @ElMassimo, PR welcome, or please create an issue if not so we can track it for v3 |
Opened #8764 so that we don't forget, thanks! 😃 |
Description
Resolves #6595 and #2375 #4198
Additional context
All assets are now included in manifest as separate entry
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).