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

prepareChunk hook #4713

Closed
patak-dev opened this issue Nov 11, 2022 · 10 comments
Closed

prepareChunk hook #4713

patak-dev opened this issue Nov 11, 2022 · 10 comments

Comments

@patak-dev
Copy link
Contributor

Feature Use Case

In Vite we are currently using our own scheme when emitting assets. Instead of import.meta.ROLLUP_FILE_URL_referenceId, we have __VITE_ASSET_referenceId__. The original reasons this was created has been resolved by:

We are exploring simplifying some of Vite's code to use more of Rollup standard paths to help with compatibility with both config options and Rollup plugins. See as an example:

I think we can't directly use import.meta.ROLLUP_FILE_URL_referenceId because we are emitting assets that are referenced in CSS files too. See for example here where we replace the __VITE_ASSET_referenceId__ in the renderChunk hook:
https://github.com/vitejs/vite/blob/main/packages/vite/src/node/plugins/asset.ts#L70

  // Urls added with JS using e.g.
  // imgElement.src = "__VITE_ASSET__5aa0ddc0__" are using quotes

  // Urls added in CSS that is imported in JS end up like
  // var inlined = ".inlined{color:green;background:url(__VITE_ASSET__5aa0ddc0__)}\n";

We also have an API to modify these asset URL references that unifies handling across JS, HTML, and CSS, and both emitted assets and public assets (a feature of Vite where files in a specific folder are copied as is to the dist folder). See docs for renderBuiltUrl here. This API is similar to resolveFileUrl.

As part of renderBuiltUrl, we are forced to recreate some of what resolveFileUrl already does. See Vite inlining some Rollup functions from here.

My current thinking is that at least for Vite 4, we aren't going to drop our current asset referencing scheme. But I'm trying to see if we should convert __VITE_ASSET__referenceId__ to import.meta.ROLLUP_FILE_URL_referenceId for JS chunks when we are generating relative URLs to avoid duplicating the above functions and to give users a way to configure the way these URLs at runtime are generated using the standard resolveFileUrl.

Feature Proposal

I think to be able to do this, we need a new prepareChunk hook that will let us process our own asset reference scheme and in some cases replace our placeholders with import.meta.ROLLUP_FILE_URL_referenceId, before resolveFileUrl is called for the chunk.

I don't know if this is enough justification for a new hook. If not, we could see if there is a way to call resolveFileUrl and the standard import.meta.ROLLUP_FILE_URL_referenceId handling on our side during renderChunk. I don't see an easy way to do this without exposing more internals from Rollup though.

@lukastaegert
Copy link
Member

So you suggest to add a hook that runs at chunk generation time that allows you to modify chunks before resolveFileUrl kicks in?

I am not sure this will work. The problem is that when we are calling resolveFileUrl, we did not search the file for references. The references were already parsed much earlier at build time. As a matter of fact, these references are handled in the AST here https://github.com/rollup/rollup/blob/master/src/ast/nodes/MetaProperty.ts#L112-L124, but we cannot put stuff into the AST after modules have been parsed, because that would be very expensive.

So my first question is: Why can't you replace these references in a transform hook that runs after all other hooks?

For assets referencing assets, this would be quite a different challenge. And maybe a chance to evolve the asset handling quite a bit more. I would be more than happy to bounce ideas with Vite here to create an API that is actually useful. So for assets, I guess replacing placeholders would be the way to go. This would happen when assets are finalized and would be straightforward as long as assets only reference assets and there are no circular references between assets.

Alternatively, we could even go one step further and align the asset and chunk hashing so that like chunks, assets only receive preliminary file names with placeholders first. Then it would be a two-pass process: When they are finalized, all file references in assets are replaced by their preliminary names, which could also includes referencing chunks from assets. Then in a second pass, we calculate the actual hashes and replace the placeholders with their final values.

To top it off, there has been the question of a transformAsset hook e.g. for multi-stage style processing (would still need a good idea how this should work) which could also tie into this. But this would also raise the questions of asset sourcemaps.

But maybe let's go back to the first question above.

@patak-dev
Copy link
Contributor Author

We can't replace these references in a transform hook because we need the chunk fileName (actually we only need the folder where it ends up) because we want to allow relative paths from the host chunk to the assets (in the same way that import.meta.ROLLUP_FILE_URL_x works). As a note, we don't always use relative paths, by default we use shorter absolute paths that may include a fixed base configured by the user. But if the user sets ./ or use the renderBuiltUrl returning { relative: true } for an asset, we compute the relative path between the chunk and the asset and apply the same new URL(...) scheme you have in Rollup.

So the idea of having a prepareChunk was being able to replace these placeholders when knowing the chunk path they are in.

This is only needed for JS chunks, and these referenced assets are standalone (don't reference any other asset). We emit assets for CSS files in some cases, and these could reference other assets but in that case, we can compute the relative path between assets before the chunking process starts.

In case it is useful, here is where we are replacing the placeholder for JS files (this runs in renderChunk currently) and here for CSS emitted assets (this runs for CSS code to get its source before doing an emitFile, also in the context of renderChunk for CSS files)

@lukastaegert
Copy link
Member

lukastaegert commented Nov 11, 2022

Of course I meant replacing them with import.meta.ROLLUP_FILE_URLx and not with the actual path. Why would this not work?

Or is the core problem that you cannot emit the file in Rollup for some reason?

@patak-dev
Copy link
Contributor Author

For some reason, I thought that our renderBuiltUrl was also giving the user the relative path as you do in resolveFileUrl. But I think you are right. We could move this code to transform (and use order: 'post'). I'll check and get back to you in a few days on this one.

@patak-dev
Copy link
Contributor Author

Ok, I tried to move our processing to a transform hook but there are two issues. First, renderBuiltUrl is given the chunk.fileName so the user can decide to do their own relative path handling. If we had a prepareChunk, the fileName at this point would have a hash placeholder. Given that we mainly want it for its folder position, we could document this and it shouldn't be an issue. We also register every asset in chunk.viteMetadata.importedAssets.

If I understand correctly, replacing our __VITE_ASSET__ placeholders in renderChunk would still properly invalidate the chunk hash when needed with the new scheme. So I think we should leave it as is. If this is the only use case for a prepareChunk I agree that it doesn't justify it.

To avoid the duplication of getRelativeUrlFromDocument in Vite (see here), maybe this function could be exported as an util from rollup?

@lukastaegert
Copy link
Member

I think the problem is that I am still not 100% sure about what you are trying to do, so let me try to paraphrase:

  • you have this renderBuiltUrl hook that you want to keep supporting
  • this hook is similar to resolveFileUrl. Actually it looks like in the case of a JavaScript chunk referencing an asset, you could just implement a resolveFileUrl hook and have that call the user's renderBuiltUrl as a kind of strangler pattern. That of course requires you to let Rollup handle the assets by emitting them.
    • Am I correct to assume that you are not ready to do that?
    • If so, is the only thing that is keeping you the ability to reference other assets from assets correctly? Or is having your own asset handling scheme crucial to Vite?
  • You originally suggested to have a hook where you replace your references with import.meta references. But this does not work because we do not do a full-text search for these references, we already know all their positions after the parsing after the last transform hook.
  • That being said, if you are not tied to the VITE_ASSET placeholder scheme, you could just as well use your own meta property and replace them in a resolveImportMeta hook instead of searching for them yourself for a potential small performance gain.
  • The main problem you need to solve is with what to replace them. As I said, if you emit your assets via Rollup, there should not be a problem, just use a transform hook to replace your VITE_ASSET references with import.meta.ROLLUP_FILE_URL references, then define a resolveFileUrl hook from which you call the renderBuiltUrl hook (at least that is how I understand it). If your problem is assets referencing other assets, then as long as there are no cycles, you can also work around this the following way:
    • emit the assets before the transform hook where they are referenced without setting their source
    • then in the renderStart hook, you call this.setAssetSource for each asset. If there are no cycles and you sort the assets accordingly, you should be able to start with assets that do no reference any other assets and continue with assets that reference only assets that already have a source. To get the file name of those assets, you can use this.getFileName. The renderStart hook is the only hook where you can do this as after that, referenced assets without a source or fixed file name will cause an error.
    • Then hook renderBuiltUrl into resolveFileUrl as you see fit

Of course we could also think about exporting something like getRelativeUrlFromDocument, but I would really hope we can work with the existing asset system or extend it to make it work.

@patak-dev
Copy link
Contributor Author

Thanks for the detailed response. I thought about using import.meta and hooking in the standard machinery. As you said, for JS, that would be ideal. The problem is that we can't use import.meta in the context of CSS assets. We are generating VITE_ASSET for both .js and .css files, and then later deciding in a renderChunk hook if the CSS should be inlined in a .js chunk or should be emitted as an asset. If it is going to end up inlined in .js, then we could use "+import.meta.(...)+", if not, we need to replace it before emitting the asset.

I'll try once more to refactor things on Vite's side. I think we could start with "+import.meta.(...)+" to interpolate in CSS and use the whole "+...+" as a placeholder in case we end up needing to emit the CSS. I also need to see if this could be done in a transform hook instead of renderChunk.

@patak-dev
Copy link
Contributor Author

Some more information after hitting a wall again. And I think this also shows that a prepareChunk hook wouldn't help. And as you said, we can't do a regex anyways there.

  • Cases we need to support for our emitted assets:
    • a. JS Host: as you proposed above, this could be implemented without issues.
    • b. CSS Host:
      • b.1 when it is emitted as an asset (.css)
      • b.2 when it is inlined as a string in a JS host
    • c. HTML Host: URLs in attributes (like src="...")

For all of these cases, we currently use the same scheme. We have __VITE_ASSET_referenceId__ (and if there is a postfix, we have __VITE_ASSET_referenceId__postfix__).

So, some issues when trying to use import.meta.ROLLUP_FILE_URL on the transform hook where we emit the assets:

  • We don't know for b. CSS Host, if we are in case b.1 or b.2 until renderChunk (we do different things depending on the build format, here. Maybe there is a chance to refactor this, but I wouldn't want to go there for Vite 4 as we want to avoid dragging the release with too big changes (and keep it focus on migrating to Rollup 3).
  • For c. HTML Host, we can't use import.meta in the attr.

I still don't see how we could unify these cases using import.meta.ROLLUP_FILE_URL, and we still need to keep both schemes, then it is easier for us to always use __VITE_ASSET_ everywhere.

I think we should close this issue, or leave it open while we discuss, this was really useful to understand what options we have already. We already made some progress to move some of the duplicated and custom handling of Vite back into Rollup with:

And we are also thinking about:

Maybe it is already enough for Vite 4.

@lukastaegert
Copy link
Member

Maybe it is already enough for Vite 4.

I think you should definitely proceed with Vite 4 at this point, I hope to do Rollup 4 a little earlier after Node 14 goes EOL next year (and probably with much fewer breaking changes), so there will be enough reason for you to do a Vite 5 in reasonable time.

I think we should definitely continue discussions about these topics, especially about assets referencing assets and the "b.2" case above. Though my feeling is, the easy solution would be to just remove the asset reference in renderChunk when inlining, so one would just need a way to also remove the asset at this point.

@patak-dev
Copy link
Contributor Author

I think next year if there aren't many breaking changes in Rollup v4, we could start work in Vite v5 a lot sooner so we can release the two with less time between them. We also plan to release a new major to drop Node 14 support. Thanks again for this discussion!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants