Deduplicated modules + emitted assets lead to an error
charlag opened this issue · 9 comments
charlag commented
Stacktrace:
Server: Error:,TypeError: Cannot read property 'replace' of undefined,TypeError: Cannot read property 'replace' of undefined
at normalizePathDelimiter (/REDACTED/tutanota-3/node_modules/nollup/lib/impl/NollupCodeGenerator.js:28:15)
at /REDACTED/tutanota-3/node_modules/nollup/lib/impl/NollupCodeGenerator.js:485:28
at Array.find (<anonymous>)
at /REDACTED/tutanota-3/node_modules/nollup/lib/impl/NollupCodeGenerator.js:484:47
at String.replace (<anonymous>)
at NollupCodeGenerator.onGenerateModulePreChunk (/REDACTED/tutanota-3/node_modules/nollup/lib/impl/NollupCodeGenerator.js:483:30)
at /REDACTED/tutanota-3/node_modules/nollup/lib/impl/NollupCompiler.js:451:37
at Array.reduce (<anonymous>)
at Object.compile (/REDACTED/tutanota-3/node_modules/nollup/lib/impl/NollupCompiler.js:447:53)
at runMicrotasks (<anonymous>)
This happens because onGenerateModulePreChunk() calls normalizePathDelimiter on each file and assets (and possibly some other files) do not have facadeModuleId. This started happening after a fix for #204 because we optimize away unnecessary chunks.
Changeing onGenerateModulePreChunk() like this seems to fix the issue:
let foundOutputChunk = bundle.find(b => {
return normalizePathDelimiter(/** @type {RollupOutputChunk} */ (b).facadeModuleId) === inner
});but I'm not sure if it's correct.
On related note, it seems like it's a very deeply nested loop (find is a loop inside replace which is called from inside of reduce(). Putting things in a map of [normalizePathDelimiter(b.facadeModuleId)]: RollupOutputFile might speed up things quite a bit.
charlag commented
I tried to do this and it seems to work but I do not see much performance impact (sorry for the messy diff, there's something up with formatting):
--- node_modules/nollup/lib/impl/NollupCodeGenerator.js 1985-10-26 09:15:00.000000000 +0100
+++ nollup-bck/lib/impl/NollupCodeGenerator.js 2021-09-07 16:23:17.043068138 +0200
@@ -6,6 +6,7 @@
const RollupConfigContainer = require('./RollupConfigContainer');
let MagicString = require('magic-string').default;
let AcornParser = require('./AcornParser');
+const {normalizePathDelimiter} = require("./utils")
/**
@@ -24,10 +25,6 @@
return input.substring(start, end).replace(/[^\n\r]/g, ' ');
}
-function normalizePathDelimiter (id) {
- return id.replace(/\\/g, '/');
-}
-
function escapeCode (code) {
// Turning the code into eval statements, so we need
// to escape line breaks and quotes. Using a multiline
@@ -474,16 +471,14 @@
/**
* @param {NollupInternalModule} file
- * @param {RollupOutputFile[]} bundle
+ * @param {Map<string, RollupOutputFile>} facadeModuleIdToModules
* @param {Object<string, NollupInternalModule>} modules
* @return {string}
*/
- onGenerateModulePreChunk (file, bundle, modules) {
+ onGenerateModulePreChunk (file, facadeModuleIdToModules, modules) {
if (file.dynamicImports.length > 0) {
return file.code.replace(/require\.dynamic\((\\)?\'(.*?)(\\)?\'\)/g, (match, escapeLeft, inner, escapeRight) => {
- let foundOutputChunk = bundle.find(b => {
- return normalizePathDelimiter(/** @type {RollupOutputChunk} */ (b).facadeModuleId) === inner
- });
+ let foundOutputChunk = facadeModuleIdToModules.get(inner);
let fileName = foundOutputChunk? foundOutputChunk.fileName : '';
return 'require.dynamic(' + (escapeLeft? '\\' : '') + '\'' + fileName + (escapeRight? '\\' : '') +'\', ' + modules[path.normalize(inner)].index + ')';
diff --color -bur node_modules/nollup/lib/impl/NollupCompiler.js nollup-bck/lib/impl/NollupCompiler.js
--- node_modules/nollup/lib/impl/NollupCompiler.js 1985-10-26 09:15:00.000000000 +0100
+++ nollup-bck/lib/impl/NollupCompiler.js 2021-09-07 16:36:07.956681780 +0200
@@ -1,7 +1,7 @@
// @ts-check
let ImportExportResolver = require('./NollupImportExportResolver');
let ParseError = require('./ParseError');
-let { getNameFromFileName, emitAssetToBundle, formatFileName, yellow } = require('./utils');
+let { getNameFromFileName, emitAssetToBundle, formatFileName, yellow, normalizePathDelimiter } = require('./utils');
let path = require('path');
let NollupContext = require('./NollupContext');
let NollupCodeGenerator = require('./NollupCodeGenerator');
@@ -26,7 +26,7 @@
Object.keys(name_map).forEach(name => {
let entries = name_map[name];
entries.forEach((entry, index) => {
- let name = entry.name + (index > 0? (index + 1) : '');
+ let name = entry.name + (index > 0 ? (index + 1) : '');
if (entry.isEntry && bundleOutputTypes[entry.facadeModuleId] === 'entry') {
if (outputOptions.file) {
@@ -147,7 +147,7 @@
context.currentModuleEmittedChunksCache = emittedChunksCache;
let loaded = await context.plugins.hooks.load(filePath, parentFilePath);
- let transformed = await context.plugins.hooks.transform( loaded.code, filePath);
+ let transformed = await context.plugins.hooks.transform(loaded.code, filePath);
let resolved = await ImportExportResolver(context.plugins, transformed.code, filePath, generator, context.liveBindings);
file.transformed = resolved.code;
@@ -205,7 +205,8 @@
for (let i = 0; i < file.imports.length; i++) {
try {
circularTrace.push(file.imports[i].source);
- await compileModule(context, file.imports[i].source, filePath, depth + 1, emitted, bundleModuleIds, generator, file.imports[i].syntheticNamedExports, false, bundleEmittedAssets, circularTrace);
+ await compileModule(context, file.imports[i].source, filePath, depth
+ + 1, emitted, bundleModuleIds, generator, file.imports[i].syntheticNamedExports, false, bundleEmittedAssets, circularTrace);
circularTrace.pop();
} catch (e) {
throw new ParseError(file.imports[i].source, e);
@@ -226,7 +227,7 @@
* @param {boolean} isEntry
* @return {Promise<Object>}
*/
-async function compileInputTarget (context, filePath, bundleModuleIds, generator, bundleEmittedAssets, isEntry) {
+async function compileInputTarget(context, filePath, bundleModuleIds, generator, bundleEmittedAssets, isEntry) {
let emitted = {
modules: {}, // modules id this input contains
dynamicImports: [], // emitted dynamic ids
@@ -242,7 +243,7 @@
await compileModule(context, filePath, parentFilePath, depth, emitted, bundleModuleIds, generator, false, isEntry, bundleEmittedAssets, [filePath]);
- function hoistDependencies (deps) {
+ function hoistDependencies(deps) {
deps.forEach(d => {
let depFile = context.files[d];
if (!depFile.hoist) {
@@ -273,7 +274,7 @@
* @param {NollupCodeGenerator} generator
* @return {Promise<NollupCompileOutput>}
*/
- async compile (context, generator) {
+ async compile(context, generator) {
context.plugins.start();
let bundle = /** @type {RollupOutputFile[]} */ ([]);
@@ -341,7 +342,7 @@
context.currentPhase = 'build';
for (let i = 0; i < context.input.length; i++) {
- let { name, file } = context.input[i];
+ let {name, file} = context.input[i];
let emitted = await compileInputTarget(context, file, bundleModuleIds, generator, bundleEmittedAssets, true);
bundle.push({
@@ -425,7 +426,7 @@
console.warn([
yellow('(!) Circular dependencies'),
...show,
- hide.length > 0? `...and ${hide.length} more` : '',
+ hide.length > 0 ? `...and ${hide.length} more` : '',
yellow('Code may not run correctly. See https://github.com/PepsRyuu/nollup/blob/master/docs/circular.md')
].join('\n'));
}
@@ -443,18 +444,25 @@
try {
await context.plugins.hooks.renderStart(context.config.output, context.config);
+ const facadeModuleIdToModules = bundle.reduce((map, file) => {
+ if (file.facadeModuleId) {
+ map.set(normalizePathDelimiter(/** @type {RollupOutputChunk} */ file.facadeModuleId), file)
+ }
+ return map
+ }, new Map())
+
// clone files and their code
modules = Object.entries(context.files).reduce((acc, val) => {
- let [ id, file ] = val;
+ let [id, file] = val;
acc[id] = {
index: file.index,
- code: generator.onGenerateModulePreChunk(file, bundle, context.files),
+ code: generator.onGenerateModulePreChunk(file, facadeModuleIdToModules, context.files),
};
return acc;
}, {});
// Rendering hooks
- let [ banner, intro, outro, footer ] = await Promise.all([
+ let [banner, intro, outro, footer] = await Promise.all([
context.plugins.hooks.banner(),
context.plugins.hooks.intro(),
context.plugins.hooks.outro(),
@@ -468,13 +476,15 @@
metaNames.forEach(metaName => {
let resolved = resolveImportMetaProperty(context.plugins, moduleId, metaName, bundleEntry, bundleReferenceIdMap);
modules[moduleId].code = modules[moduleId].code.replace(
- metaName === null? new RegExp('import\\.meta') : new RegExp('import\\.meta\\.' + metaName, 'g'),
+ metaName === null ? new RegExp('import\\.meta') : new RegExp('import\\.meta\\.' + metaName, 'g'),
resolved
);
});
});
- bundleEntry.code = banner + '\n' + intro + '\n' + generator.onGenerateChunk(modules, bundleEntry, context.config.output, context.config) + '\n' + outro + '\n' + footer;
+ bundleEntry.code = banner + '\n' + intro + '\n'
+ + generator.onGenerateChunk(modules, bundleEntry, context.config.output, context.config) + '\n' + outro + '\n'
+ + footer;
await context.plugins.hooks.renderChunk(bundleEntry.code, bundleEntry, context.config.output);
}
@@ -503,7 +513,7 @@
})));
return {
- stats: { time: Date.now() - bundleStartTime },
+ stats: {time: Date.now() - bundleStartTime},
changes: changes,
output: bundle
}
diff --color -bur node_modules/nollup/lib/impl/utils.js nollup-bck/lib/impl/utils.js
--- node_modules/nollup/lib/impl/utils.js 1985-10-26 09:15:00.000000000 +0100
+++ nollup-bck/lib/impl/utils.js 2021-09-07 16:35:25.727720346 +0200
@@ -102,6 +102,9 @@
bundle.push(bundleEntry);
}
+function normalizePathDelimiter (id) {
+ return id.replace(/\\/g, '/');
+}
module.exports = {
@@ -111,5 +114,6 @@
formatFileName,
getNameFromFileName,
emitAssetToBundle,
- findChildNodes
+ findChildNodes,
+ normalizePathDelimiter,
};
\ No newline at end of file
PepsRyuu commented
So I understand the fix, but I'm trying to understand how to replicate this as a test case. I'm probably missing something obvious, but how do you trigger a chunk without a facadeModuleId? What is this optimisation process?
charlag commented
So I understand the fix, but I'm trying to understand how to replicate this as a test case. I'm probably missing something obvious, but how do you trigger a chunk without a
facadeModuleId? What is this optimisation process?
You can produce such a chunk with
this.emitFile({
type: 'asset',
name,
fileName: name,
source: content,
})inside a plugin
PepsRyuu commented
Ah I'm not filtering out assets when checking. That makes sense! So the test needs to emit an asset first, then emit a chunk to trigger the issue. Will look into this! Much appreciated!
charlag commented
Thank you!
PepsRyuu commented
Released in 0.18.4
charlag commented
Thanks! I still think that calling find in a deeply nested loop is not very performant though.
PepsRyuu commented
Probably not the most efficient no, but it can be something addressed as a separate issue outside of this bug, as I think there's lots of room for improvement across the board for performance. Are you finding drastically improved performance with the alternative solution?
charlag commented
You are right.
From my tests I didn't notice much difference but as it is squared complexity it can get bad really quick if you are unlucky. I wanted to take V8 performance profile but didn't find time to do it yet.