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

augmentChunkHash plugin hook #2921

Merged
merged 16 commits into from Aug 13, 2019
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/Chunk.ts
Expand Up @@ -124,6 +124,7 @@ export default class Chunk {
return chunk;
}

augmentedHash?: string;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From how this is generated, I would say this is more like a hashAugmentation as the actual hash is composed of more things. But maybe we can get rid of this property, cf. my other comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense and will actually accomplish the lazy behaviour

entryModules: Module[] = [];
execIndex: number;
exportMode = 'named';
Expand Down Expand Up @@ -325,6 +326,9 @@ export default class Chunk {
if (this.renderedHash) return this.renderedHash;
if (!this.renderedSource) return '';
const hash = sha256();
if (this.augmentedHash) {
hash.update(this.augmentedHash);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the renderedHash is effectively cached and determined at most once, but sometimes not at all, i.e. if there is no [hash] in the name pattern, I wondered if it would not be better to generate this value on the fly. The only necessary parameter graph is available on each chunk anyway and we would not need the additional property on the Chunk class.

}
hash.update(this.renderedSource.toString());
hash.update(
this.getExportNames()
Expand Down Expand Up @@ -799,7 +803,6 @@ export default class Chunk {

private computeContentHashWithDependencies(addons: Addons, options: OutputOptions): string {
const hash = sha256();

hash.update(
[addons.intro, addons.outro, addons.banner, addons.footer].map(addon => addon || '').join(':')
);
Expand Down
109 changes: 73 additions & 36 deletions src/rollup/index.ts
Expand Up @@ -22,6 +22,7 @@ import {
OutputOptions,
Plugin,
PluginContext,
PreRenderedChunk,
RollupBuild,
RollupCache,
RollupOutput,
Expand Down Expand Up @@ -211,45 +212,81 @@ export default function rollup(rawInputOptions: GenericConfigObject): Promise<Ro
optimized = true;
}

assignChunkIds(chunks, inputOptions, outputOptions, inputBase, addons);

// assign to outputBundle
for (let i = 0; i < chunks.length; i++) {
const chunk = chunks[i];
const facadeModule = chunk.facadeModule;

outputBundle[chunk.id] = {
code: undefined as any,
dynamicImports: chunk.getDynamicImportIds(),
exports: chunk.getExportNames(),
facadeModuleId: facadeModule && facadeModule.id,
fileName: chunk.id,
imports: chunk.getImportIds(),
isDynamicEntry:
facadeModule !== null && facadeModule.dynamicallyImportedBy.length > 0,
isEntry: facadeModule !== null && facadeModule.isEntryPoint,
map: undefined,
modules: chunk.renderedModules,
get name() {
return chunk.getChunkName();
}
} as OutputChunk;
}

return Promise.all(
chunks.map(chunk => {
const outputChunk = outputBundle[chunk.id] as OutputChunk;
return chunk.render(outputOptions, addons, outputChunk).then(rendered => {
outputChunk.code = rendered.code;
outputChunk.map = rendered.map;

return graph.pluginDriver.hookParallel('ongenerate', [
{ bundle: outputChunk, ...outputOptions },
outputChunk
]);
});
const facadeModule = chunk.facadeModule;
const chunkInfo = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not like calculating this information twice. Besides performance considerations, this information will need to be maintained and synced in more than one place which is always problematic.

One way forward might be to first create a Map of Chunk -> PreRenderedChunk info and use this map in the second usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, could also do it in an array and just match positions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would be fine as well.

dynamicImports: chunk.getDynamicImportIds(),
exports: chunk.getExportNames(),
facadeModuleId: facadeModule && facadeModule.id,
imports: chunk.getImportIds(),
isDynamicEntry:
facadeModule !== null && facadeModule.dynamicallyImportedBy.length > 0,
isEntry: facadeModule !== null && facadeModule.isEntryPoint,
modules: chunk.renderedModules,
get name() {
return chunk.getChunkName();
}
} as PreRenderedChunk;
return graph.pluginDriver
.hookReduceValue(
'augmentChunkHash',
'',
[chunkInfo],
(hashForChunk, pluginHash) => {
if (pluginHash) {
hashForChunk += pluginHash;
}
return hashForChunk;
}
)
.then(hashForChunk => {
if (hashForChunk) {
chunk.augmentedHash = hashForChunk;
}
});
})
).then(() => {});
).then(() => {
assignChunkIds(chunks, inputOptions, outputOptions, inputBase, addons);

// assign to outputBundle
for (let i = 0; i < chunks.length; i++) {
const chunk = chunks[i];
const facadeModule = chunk.facadeModule;

outputBundle[chunk.id] = {
code: undefined as any,
dynamicImports: chunk.getDynamicImportIds(),
exports: chunk.getExportNames(),
facadeModuleId: facadeModule && facadeModule.id,
fileName: chunk.id,
imports: chunk.getImportIds(),
isDynamicEntry:
facadeModule !== null && facadeModule.dynamicallyImportedBy.length > 0,
isEntry: facadeModule !== null && facadeModule.isEntryPoint,
map: undefined,
modules: chunk.renderedModules,
get name() {
return chunk.getChunkName();
}
} as OutputChunk;
}

return Promise.all(
chunks.map(chunk => {
const outputChunk = outputBundle[chunk.id] as OutputChunk;
return chunk.render(outputOptions, addons, outputChunk).then(rendered => {
outputChunk.code = rendered.code;
outputChunk.map = rendered.map;

return graph.pluginDriver.hookParallel('ongenerate', [
{ bundle: outputChunk, ...outputOptions },
outputChunk
]);
});
})
).then(() => {});
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what exactly causes the test failures, e.g. misc/'handles different import paths for different outputs', but it is definitely caused by some of the changes in this file. Maybe it becomes clearer once the code is deduplicated. My guess is that there is something that has a side effect that does not look side-effectful here.

Copy link
Contributor Author

@isidrok isidrok Jul 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have found what's causing the errors and its pretty strange:

return graph.pluginDriver
  .hookParallel('renderStart', [])
  .then(() => createAddons(graph, outputOptions))
  .then(addons => {
    // ...
    return Promise.all(
      chunks.map(chunk => {
        const outputChunk = outputBundle[chunk.id] as OutputChunk;
        return chunk.render(outputOptions, addons, outputChunk).then(rendered => {
          // ...  
          return graph.pluginDriver.hookParallel('ongenerate', [
            { bundle: outputChunk, ...outputOptions },
           outputChunk
          ]);
        });
      })
    ).then(() => {});
})

Works fine and test pass whereas if I move the Promise.all block into another promise:

return graph.pluginDriver
  .hookParallel('renderStart', [])
  .then(() => createAddons(graph, outputOptions))
  .then(addons => {
    // ...
    const augmentChunkHashes = Promise.resolve();
    return augmentChunkHash.then( () => {
      // Moving this Promise.all inside a then breaks test
      return Promise.all(
        chunks.map(chunk => {
          const outputChunk = outputBundle[chunk.id] as OutputChunk;
          return chunk.render(outputOptions, addons, outputChunk).then(rendered => {
            // ...  
            return graph.pluginDriver.hookParallel('ongenerate', [
              { bundle: outputChunk, ...outputOptions },
              outputChunk
            ]);
          });
        })
      ).then(() => {});
   })
})

Some tests break but if I understand correctly:

Promise.all(promises)
  .then(()=>{}) 
=== 
Promise.resolve()
  .then(()=>Promise.all(promises).then(()=>{}))

Any idea?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a slight difference in that errors thrown synchronously when generating the promises will be caught and reject the Promise in the second case while in the first case, they will be thrown synchronously. But I do not think this is the issue here.

I cannot seem to be able to confirm your finding, will do some digging myself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also in the second case, execution will be one micro-tick slower, which would point to a possible race condition. But I really hope this is not the issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I understand the issue now. It could be argued that this is a bug in Rollup but it is actually only a bug with your modification.
The problem is that the core part of chunk rendering is stateful, i.e. you cannot just interleave the renderings of two different chunks as they leave information about the current rendering process at variables etc. I know this is not ideal but for the time being, I fear we will need to live with this.

The state only needs to be preserved between chunk.preRender and chunk.render. Previously, this was guaranteed but with your modification, there is now an asynchronous micro-tick between those two calls where another output rendering can sneak in. Which is a problem as the CLI does indeed use Promise.all to render outputs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly not sure what a good solution is except forbidding augmentChunkHash to be asynchronous for now.

Copy link
Contributor Author

@isidrok isidrok Jul 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this, I was loosing my mind. So if I get it right the correct order of events would be to preRender and then render all chunks for each output but with the constraint that this sequence has to be completed before another output goes in. Moving the rendering to another micro-task makes it possible for some sequences when several outputs are generated to go preRender-ChunkA-OutA preRender-ChunkA-OutB render-ChunkA-OutA or something among those lines breaking the sequence since they are generated inside a Promise.all in parallel.

Another possible solution could be running this hook before generate but I would rather have the information generated during preRender and make this syncrhonous with a new hookReduceValueSync hook for example.

})
.catch(error =>
graph.pluginDriver.hookParallel('renderError', [error]).then(() => {
Expand Down
8 changes: 6 additions & 2 deletions src/rollup/types.d.ts
Expand Up @@ -276,6 +276,7 @@ interface OnWriteOptions extends OutputOptions {
}

export interface PluginHooks {
augmentChunkHash: (this: PluginContext, chunk: PreOutputChunk) => void;
buildEnd: (this: PluginContext, err?: Error) => Promise<void> | void;
buildStart: (this: PluginContext, options: InputOptions) => Promise<void> | void;
generateBundle: (
Expand Down Expand Up @@ -446,11 +447,10 @@ export interface RenderedModule {
renderedLength: number;
}

export interface RenderedChunk {
export interface PreRenderedChunk {
isidrok marked this conversation as resolved.
Show resolved Hide resolved
dynamicImports: string[];
exports: string[];
facadeModuleId: string | null;
fileName: string;
imports: string[];
isDynamicEntry: boolean;
isEntry: boolean;
Expand All @@ -460,6 +460,10 @@ export interface RenderedChunk {
name: string;
}

export interface RenderedChunk extends PreRenderedChunk {
fileName: string;
}

export interface OutputChunk extends RenderedChunk {
code: string;
map?: SourceMap;
Expand Down
153 changes: 153 additions & 0 deletions test/hooks/index.js
Expand Up @@ -1328,4 +1328,157 @@ module.exports = input;
]);
});
});
it('supports augmentChunkHash hook', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be interesting to convert some of those tests to file-hashes tests, i.e. tests/file-hashes/samples/.... These are specifically designed to compare two different setups and throw when there are files with the same hashes but different content. Also, those tests are closer to real world scenarios.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

let augmentChunkHashCalls = 0;
return rollup
.rollup({
input: 'input',
plugins: [
loader({
input: `alert('hello')`
}),
{
augmentChunkHash(update) {
augmentChunkHashCalls++;
assert(this.meta);
assert(this.meta.rollupVersion);
}
}
]
})
.then(bundle =>
bundle.generate({
format: 'esm',
dir: 'dist',
entryFileNames: '[name]-[hash].js'
})
)
.then(output => {
assert.equal(augmentChunkHashCalls, 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think this is too important to test as calling is implicitly tested with any test that also tests that the hash is changed, which is what is actually important to us.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the end I left this one inside hook tests and created a file-hashes one with tests the rest of them

});
});
it('augments the chunk hash when the update function is called within the augmentChunkHash hook', () => {
const inputCode = `alert('hello')`;
const outputOptions = {
format: 'esm',
dir: 'dist',
entryFileNames: '[name]-[hash].js'
};
function getFileName(bundle) {
return bundle.generate(outputOptions).then(({ output }) => {
return output[0].fileName;
});
}
function bundleWithoutAugment() {
return rollup
.rollup({
input: 'input',
plugins: [
loader({
input: inputCode
})
]
})
.then(getFileName);
}
function bundleWithAugment() {
return rollup
.rollup({
input: 'input',
plugins: [
loader({
input: inputCode
}),
{
augmentChunkHash() {
return 'foo';
}
}
]
})
.then(getFileName);
}
return Promise.all([bundleWithoutAugment(), bundleWithAugment()]).then(([base, augmented]) => {
assert.notEqual(base, augmented);
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is a prime candidate for being converted to a file-hashes test.

});
it('propagates hash updates of augmentChunkHash to all dependents', () => {
return rollup
.rollup({
input: 'input',
plugins: [
loader({
input: `console.log('input');import('other');`,
other: `console.log('other');`
}),
{
augmentChunkHash() {
return Promise.resolve('foo');
}
}
]
})
.then(bundle =>
bundle.generate({
format: 'esm',
dir: 'dist',
entryFileNames: '[name]-[hash].js',
chunkFileNames: '[name]-[hash].js'
})
)
.then(({ output }) => {
const importedFileName = output[0].dynamicImports[0];
const otherFileName = output[1].fileName;
assert.equal(otherFileName, importedFileName);
});
});
it('augmentChunkHash only takes effect for chunks whose call got a return value', () => {
const outputOptions = {
format: 'esm',
dir: 'dist',
entryFileNames: '[name]-[hash].js'
};
const input = ['input', 'other'];
const inputCode = {
input: `console.log('input');`,
other: `console.log('other');`
};
function getFileNamesForChunks(bundle) {
return bundle.generate(outputOptions).then(({ output }) => {
return output.reduce((result, chunk) => {
result[chunk.name] = chunk.fileName;
return result;
}, {});
});
}
function bundleWithoutAugment() {
return rollup
.rollup({
input,
plugins: [loader(inputCode)]
})
.then(getFileNamesForChunks);
}
function bundleWithAugment() {
return rollup
.rollup({
input,
plugins: [
loader(inputCode),
{
augmentChunkHash(chunk) {
if (chunk.name === 'input') {
return 'foo';
}
}
}
]
})
.then(getFileNamesForChunks);
}
return Promise.all([bundleWithoutAugment(), bundleWithAugment()]).then(([base, augmented]) => {
assert.notEqual(base.input, augmented.input);
assert.equal(base.other, augmented.other);
});
});
});