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

A method of adding an import based on dependency graph #3790

Closed
benmccann opened this issue Sep 18, 2020 · 7 comments · Fixed by #4543 or #4549
Closed

A method of adding an import based on dependency graph #3790

benmccann opened this issue Sep 18, 2020 · 7 comments · Fixed by #4543 or #4549

Comments

@benmccann
Copy link
Contributor

Feature Use Case

Sapper has a CSS-handling plugin, which has recently been significantly cleaned up and is about ready to graduate into a separate Rollup plugin for wider community use:

https://github.com/sveltejs/sapper/blob/ead10eebb7416971a390a0190f1df7c1f3327791/src/core/create_compilers/RollupCompiler.ts#L156

It finds CSS that a dynamically imported chunk tree includes and then rewrites the dynamic imports with an extra bit of code. That code makes it so that in parallel to the original dynamic import we also write link tags pointing to the CSS.

So as not to duplicate code across chunks, this plugin emits a new inject_styles chunk. It then adds an import to that chunk where it's used. The problem here is that this import does not show up in the chunk.imports for the plugins that follow it.

Feature Proposal

Things I've tried:

  • I've tried doing chunk.imports.push(newImport) when adding the import in generateBundle. This does not persist for the next plugin
  • I've tried rewriting the code in transform. This does not work because I don't have access to the dependency graph. When I call this.getModuleInfo it returns empty arrays for the importedIds and dynamicallyImportedIds
  • I've tried rewriting the code in renderChunk. However, I don't know the file name to add. If I simply do import __inject_styles from 'inject_styles.js' then that doesn't get properly rewritten to include the hash in the filename.

It would be nice if at least one of these worked or there was another solution to this problem

Related Requests

#3519

@codepunkt
Copy link

I've been running into an issue that is kinda similar. Did you have any success?

@lukastaegert
Copy link
Member

I've tried rewriting the code in transform. This does not work because I don't have access to the dependency graph. When I call this.getModuleInfo it returns empty arrays for the importedIds and dynamicallyImportedIds

Sorry for not getting back to you when this issue was created. A lot has happened since then, especially the introduction of the this.load context function. I have recently put some work into streamlining the process of scanning dependency graphs inside build time hooks, maybe #4358 will allow you to solve the issue?

@benmccann
Copy link
Contributor Author

Sapper has been replaced by SvelteKit, which is built on top of Vite, so I no longer have a direct need for this. I see a few folks from the Vite team copied on #4358. As far as I'm concerned, I'm happy with any solution they're happy with, so feel free to link this issue to that PR so that it will be closed by it

@lukastaegert
Copy link
Member

#4543 now allows you (even encourages you) to update the import/export information when updating a chunk

@rollup-bot
Copy link
Collaborator

This issue has been resolved via #4543 as part of rollup@3.0.0-7. Note that this is a pre-release, so to test it, you need to install Rollup via npm install rollup@3.0.0-7 or npm install rollup@beta. It will likely become part of a regular release later.

@rollup-bot
Copy link
Collaborator

This issue has been resolved via #4543 as part of rollup@3.0.0-8. Note that this is a pre-release, so to test it, you need to install Rollup via npm install rollup@3.0.0-8 or npm install rollup@beta. It will likely become part of a regular release later.

@rollup-bot
Copy link
Collaborator

This issue has been resolved via #4543 as part of rollup@3.0.0. You can test it via npm install rollup.

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