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(dev): added assets to manifest #6649

Merged
merged 13 commits into from Jun 19, 2022

Conversation

lubomirblazekcz
Copy link
Contributor

@lubomirblazekcz lubomirblazekcz commented Jan 26, 2022

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?

  • 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 Commit 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.

@lubomirblazekcz
Copy link
Contributor Author

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 .js. I didn't find where could I change this.

This doesn't happend with .css, other static assets should also copy to dist if included as input entrypoint. It's important for backend integration, beucase there can be static assets paths in templates (.twig, .blade, .latte, etc.) and there is no way to reslove them on backend if they are not processed correctly and part of the manifest.

Generated an empty chunk: "fragment.svg"
Generated an empty chunk: "icon.png"

@patak-dev patak-dev added the p2-nice-to-have Not breaking anything but nice to have (priority) label Feb 14, 2022
@innocenzi
Copy link
Contributor

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
Copy link
Contributor

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.

Copy link
Contributor Author

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

@lubomirblazekcz
Copy link
Contributor Author

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.

@lubomirblazekcz
Copy link
Contributor Author

@innocenzi @patak-dev any chance getting this into v3? I think this would also solve #2375 and #4198

@innocenzi
Copy link
Contributor

Would love this PR to be merged, but I think it's not completely done yet?

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 .js. I didn't find where could I change this.

@lubomirblazekcz
Copy link
Contributor Author

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.

@lubomirblazekcz
Copy link
Contributor Author

lubomirblazekcz commented May 28, 2022

@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

@patak-dev patak-dev added p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) and removed p2-nice-to-have Not breaking anything but nice to have (priority) labels Jun 3, 2022
@jessarcher
Copy link
Contributor

Thanks for this PR!

I'm concerned about chunk.name just being the file name and not the full path to the source asset.

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.

@lubomirblazekcz
Copy link
Contributor Author

@jessarcher in my tests chunk.name returns whole path relative to entrypoint. At least it returns the path same way as for JS. Also filename collisions shoudn't occur, because the path is relative to entrypoint.

For reference, this is from my tests in playground/backend-integration

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"
  }
}

@jessarcher
Copy link
Contributor

jessarcher commented Jun 8, 2022

@lubomirblazekcz I think that's because in the test, root has been specified as frontend/entrypoints.

If the root was not a subdirectory, then main.ts would be frontend/entrypoints/main.ts, but global.css would remain global.css.

This is because for the "chunk" type, it uses getChunkName(), which looks at chunk.facadeModuleId rather than chunk.name. Unfortunately, chunk.facadeModuleId doesn't exist on "asset" chunks.

In css.ts, the full CSS chunk is replaced in the bundle with an asset chunk.

In userland, the only way I can find to get the path is by grabbing it in the renderChunk hook where the original chunk is still present with the facadeModuleId.

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 global.css is an entrypoint, but it's also being imported in index.html, so I might try to tweak the tests a bit to separate the CSS entry point from the imported CSS.

@jessarcher
Copy link
Contributor

I've tried making the following change in css.ts:

- 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 playground/backend-integration).

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 file is a JS file (?!). Additionally, index.html loses its css key, with the CSS being moved to the imports key instead. The latter may not be an issue in practice because it seems weird to import a CSS file that's also an entry point.

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 removedPureCssFilesCache so I could import that in manifest.ts to append to the manifest, but that seems like a hack.

@lubomirblazekcz
Copy link
Contributor Author

@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.

@jessarcher
Copy link
Contributor

jessarcher commented Jun 8, 2022

@lubomirblazekcz Oh, I didn't notice it was only a problem with CSS!

After looking at asset.ts, something like this in css.ts seems to solve it when combined with your PR!

  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 🙂

@lubomirblazekcz
Copy link
Contributor Author

@jessarcher thanks that indeed fixed the issue! But it broke a bunch of tests, I will have to investigate further what went wrong :/

@lubomirblazekcz
Copy link
Contributor Author

It broke playground/assets, for some reason index.html is now treated as asset and not entry, so it doesn't include info about assets.

@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.

Comment on lines 516 to 524
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]')

Copy link
Contributor

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.

Copy link
Member

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

@lubomirblazekcz
Copy link
Contributor Author

@jessarcher great work! Everything seems to work good as expected now 👍

@patak-dev
Copy link
Member

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 patak-dev added this to the 3.0 milestone Jun 10, 2022
@lubomirblazekcz
Copy link
Contributor Author

@patak-dev schema for manifests sounds like a great idea, it's something that for sure can be handy in the future

@bholmesdev
Copy link
Contributor

bholmesdev commented Jun 17, 2022

This was a major blocker for Slinkity and Astro as we explore a special assets directory for SSR assets. I currently generate a virtual module to "spoof" imports just to find the input file for a given CSS output file. Assuming this gives me an input-ouput mapping for assets... it sounds like a godsend 🙏

@patak-dev
Copy link
Member

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.

@innocenzi
Copy link
Contributor

This is fantastic news, thanks @patak-dev!

@ElMassimo
Copy link
Contributor

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 .ts extensions:

  "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?

@patak-dev
Copy link
Member

Not yet @ElMassimo, PR welcome, or please create an issue if not so we can track it for v3
Thanks for testing!

@ElMassimo
Copy link
Contributor

Opened #8764 so that we don't forget, thanks! 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[backend-integration] Include CSS and other assets in manifest
6 participants