draft for extended renderDynamicImport hook #5870
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
8451962 to
3e0f0d3
Compare
|
Test - actual
+ expected
const asset$1 = 'resolved';
const chunk$1 = 'resolved';
const asset = new URL('assets/asset-unresolved-B7Qh6_pN.txt', module.meta.url).href;
- const chunk = new URL('nested/chunk.js', module.meta.url).href;
+ const chunk = new URL('nested/chunk2.js', module.meta.url).href;
- module.import('./nested/chunk2.js').then(result => console.log(result, chunk$1, chunk, asset$1, asset));
+ module.import('./nested/chunk.js').then(result => console.log(result, chunk$1, chunk, asset$1, asset));
})
};
}));I believe it is because I moved Edit: fixed, mostly |
There was a problem hiding this comment.
Hi, this is a really good idea! Some impressions first:
- Why did you put the logic into the "render" method, and why did you create an interface where the plugin returns a function to be called again with the import info? I would have hoped we can just put this information into the argument of renderDynamicImport?
- If this is about performance and you only want to calculate the paths if needed, we could also think about other solutions, e.g. to give the plugin all the information to calculate the relative path on their own? Then we could just add e.g.
chunk.getPreRenderedChunkInfo()astargetChunkto the options argument in therenderDynamicImporthook, and put this information into the PreRenderedChunk type as well (this is already cached). The importing chunk could also be added a achunk: PreRenderedChunkargument. - It would be nice to update src/rollup/types.d.ts to contain the new arguments.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5870 +/- ##
=======================================
Coverage 98.54% 98.55%
=======================================
Files 269 269
Lines 8550 8563 +13
Branches 1467 1470 +3
=======================================
+ Hits 8426 8439 +13
Misses 92 92
Partials 32 32 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi, thanks for the review.
Regarding this comment #5870 (comment), currently the patch does include hoisted transitive dependencies in the object it passes to the hook (please see https://github.com/rollup/rollup/pull/5870/files#diff-665b5d9bb97c88c8f65a65f1904d1742223d88909e8978eac1c186d6749b6a98, specifically |
d24b286 to
1b3652d
Compare
1b3652d to
6be03e8
Compare
6be03e8 to
9180e54
Compare
test/chunking-form/samples/render-dynamic-import-extended/_config.js
Outdated
Show resolved
Hide resolved
|
I've changed |
86bd0a0 to
ee6721b
Compare
|
Thank you as well for the excellent bundler! |
|
This PR has been released as part of rollup@4.36.0. You can test it via |
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
When using dynamic imports, there is the parent module (the module containing the
import()expression), the target module (whatever got imported by the dynamic import), and optionally also any transitively imported modules (anything that is imported by the target module viaimportstatement). Sometimes, transitively imported modules will be included in a separate chunk from the target module. In this case, the dynamic import will require the equivalent of two network round trips before completing, because the transitive import will not be loaded until after the target of the original dynamic import is loaded.For "static" import statements, there seems to be a feature (
hoistTransitiveImports) that moves transitive imports upwards. In the case of an import chainA -> B -> C, assuming all 3 modules are bundled into separate chunks, chunk A will contain an import statement for chunk C to avoid an extra RTT. Unfortunately, this currently does not appear to be directly possible for dynamic imports, as chunk information is not available in therenderDynamicImporthook. By exposing chunk import information inrenderDynamicImport, plugins can easily insert a call to import or preload transitively imported chunks at the dynamic import site.Todo:
Imports added byhoistTransitiveImportscurrently don't appear in the imports list, which defeats the entire purpose of this PRImport paths are not resolved