-
Notifications
You must be signed in to change notification settings - Fork 28
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
Deduplicated modules + emitted assets lead to an error #212
Comments
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
|
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 |
You can produce such a chunk with this.emitFile({
type: 'asset',
name,
fileName: name,
source: content,
}) inside a plugin |
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! |
Thank you! |
Released in |
Thanks! I still think that calling |
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? |
You are right. |
Stacktrace:
This happens because
onGenerateModulePreChunk()
calls normalizePathDelimiter on each file and assets (and possibly some other files) do not havefacadeModuleId
. This started happening after a fix for #204 because we optimize away unnecessary chunks.Changeing
onGenerateModulePreChunk()
like this seems to fix the issue: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 insidereplace
which is called from inside ofreduce()
. Putting things in a map of[normalizePathDelimiter(b.facadeModuleId)]: RollupOutputFile
might speed up things quite a bit.The text was updated successfully, but these errors were encountered: