-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Create chunks for virtual modules when preserving modules #2511
Conversation
ca90a85
to
fcf68aa
Compare
8b20bcd
to
9d26dd2
Compare
2601256
to
df0caff
Compare
…g module order test
df0caff
to
dbbd52d
Compare
9d26dd2
to
f505bfd
Compare
Damn it, seems like Windows really does not like my tests. Will keep on investigating tomorrow. |
description: 'Generates actual files for virtual modules when preserving modules', | ||
options: { | ||
input: ['main.js'], | ||
experimentalCodeSplitting: true, |
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.
arent those like conflicting settings? preserving modules & code splitting?
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.
Not in rollup < 1.0. experimentalCodeSplitting
internally uses the "new" code-splitting pipeline which is a prerequisite for preserving modules, which is like the ultimate form of code-splitting. In rollup 1.0 of course, this is the only pipeline that exists.
.generate({ format: 'esm' }) | ||
.then(generated => | ||
assert.deepEqual(Object.keys(generated.output), [ | ||
'ROLLUP__virtualModule', |
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.
wouldnt it be better to generate hashed 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.
I tend to disagree here. Two reasons:
- It is unclear how the file name should be generated.
- The point of preserving modules is to preserve as much as possible. Nothing else is hashed and thus there is no benefit in hashing just this file. On the contrary, this will cause friction as it is not a priori clear which file names will be generated.
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.
Looks good!
src/Chunk.ts
Outdated
this.id = normalize( | ||
isAbsolute(this.entryModule.id) | ||
? relative(preserveModulesRelativeDir, sanitizedId) | ||
: 'ROLLUP_' + basename(sanitizedId) |
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.
Perhaps make these a folder - _virtual/...
, or something like that, in case there are a lot of them?
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.
Good idea!
const inputBase = commondir( | ||
chunks.filter(chunk => chunk.entryModule).map(chunk => chunk.entryModule.id) | ||
chunks | ||
.filter(chunk => chunk.entryModule && isAbsolute(chunk.entryModule.id)) |
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.
Doesn't the ID generation for preserve modules now guarantee this is always true?
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.
Not if the id is generated by a plugin. And this extends to other workflows as well.
@@ -2,6 +2,7 @@ import * as path from './path'; | |||
|
|||
// ported from https://github.com/substack/node-commondir | |||
export default function commondir(files: string[]) { | |||
if (files.length === 0) return '/'; |
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.
Sehr gut.
ba6cf4e
to
4abb2e0
Compare
4abb2e0
to
b68ce59
Compare
I went for the |
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Resolves #2503
Description
When preserving modules, we also need to create chunks for virtual modules injected by plugins (e.g. babel helpers etc.). This PR solves this by doing the following:
\0
characters are replaced with_
ROLLUP_
to reduce the danger of name clashes and turned into top-level modules (there is no check if such an id is already taken, though, therefore the prefix)