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

Fixing extra and invalid paths in Webpack 5 #249

Merged
merged 4 commits into from
Apr 1, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 45 additions & 23 deletions lib/helpers.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
const { dirname, join, basename } = require('path');

const generateManifest = (compilation, files, { generate, seed = {} }) => {
let result;
if (generate) {
Expand Down Expand Up @@ -25,7 +27,13 @@ const getFileType = (fileName, { transformExtensions }) => {
};

const reduceAssets = (files, asset, moduleAssets) => {
const name = moduleAssets[asset.name] ? moduleAssets[asset.name] : asset.info.sourceFilename;
let name;
if (moduleAssets[asset.name]) {
name = moduleAssets[asset.name];
} else if (asset.info.sourceFilename) {
name = join(dirname(asset.name), basename(asset.info.sourceFilename));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is basically the same thing that's already being done at the bottom of hooks.js for the file-loader/ NormalModule situation -

Object.assign(moduleAssets, { [file]: join(dirname(file), basename(module.userRequest)) });

}

if (name) {
return files.concat({
path: asset.name,
Expand All @@ -52,29 +60,43 @@ const reduceAssets = (files, asset, moduleAssets) => {
});
};

const reduceChunk = (files, chunk, options) =>
Array.of(...Array.from(chunk.files), ...Array.from(chunk.auxiliaryFiles || [])).reduce(
(prev, path) => {
let name = chunk.name ? chunk.name : null;
// chunk name, or for nameless chunks, just map the files directly.
name = name
? options.useEntryKeys && !path.endsWith('.map')
? name
: `${name}.${getFileType(path, options)}`
: path;
const reduceChunk = (files, chunk, options, auxiliaryFiles) => {
// auxiliary files contain things like images, fonts AND, most
// importantly, other files like .map sourcemap files
// we modify the auxiliaryFiles so that we can add any of these
// to the manifest that was not added by another method
// (sourcemaps files are not added via any other method)
Array.from(chunk.auxiliaryFiles || []).forEach((auxiliaryFile) => {
auxiliaryFiles[auxiliaryFile] = {
path: auxiliaryFile,
name: basename(auxiliaryFile),
isInitial: false,
isChunk: false,
isAsset: true,
isModuleAsset: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe these are all correct: these are not chunks, they are assets. And I believe they are "module assets" because they are auxiliary assets to a chunk, but I'm not positive if that is the intention. These will most notably include sourcemap files.

};
});

return Array.from(chunk.files).reduce((prev, path) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you view this section without whitespace - https://github.com/shellscape/webpack-manifest-plugin/pull/249/files?w=1 - you'll see that the existing logic remains the same. The chunk.auxiliaryFiles were removed from the "normal" logic and moved into the above loop to track those custom.

let name = chunk.name ? chunk.name : null;
// chunk name, or for nameless chunks, just map the files directly.
name = name
? options.useEntryKeys && !path.endsWith('.map')
? name
: `${name}.${getFileType(path, options)}`
: path;

return prev.concat({
path,
chunk,
name,
isInitial: chunk.isOnlyInitial(),
isChunk: true,
isAsset: false,
isModuleAsset: false
});
},
files
);
return prev.concat({
path,
chunk,
name,
isInitial: chunk.isOnlyInitial(),
isChunk: true,
isAsset: false,
isModuleAsset: false
});
}, files);
};

const standardizeFilePaths = (file) => {
const result = Object.assign({}, file);
Expand Down
14 changes: 13 additions & 1 deletion lib/hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,9 @@ const emitHook = function emit(

emitCountMap.set(manifestFileName, emitCount);

const auxiliaryFiles = {};
let files = Array.from(compilation.chunks).reduce(
(prev, chunk) => reduceChunk(prev, chunk, options),
(prev, chunk) => reduceChunk(prev, chunk, options, auxiliaryFiles),
[]
);

Expand All @@ -66,6 +67,17 @@ const emitHook = function emit(
typeof emitCountMap.get(join(compiler.options.output.path, name)) === 'undefined'
);

// auxiliary files are "extra" files that are probably already included
// in other ways. Loop over files and remove any from auxiliaryFiles
files.forEach((file) => {
delete auxiliaryFiles[file.path];
});
// if there are any auxiliaryFiles left, add them to the files
// this handles, specifically, sourcemaps
Object.keys(auxiliaryFiles).forEach((auxiliaryFile) => {
files = files.concat(auxiliaryFiles[auxiliaryFile]);
});

files = files.map((file) => {
const changes = {
// Append optional basepath onto all references. This allows output path to be reflected in the manifest.
Expand Down
6 changes: 3 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@
"lint:js": "eslint --fix --cache lib test",
"lint:json": "prettier --write codecov.yml .circleci/config.yml .eslintrc",
"lint:package": "prettier --write package.json --plugin=prettier-plugin-package",
"posttest": "npm install webpack@^4.44.2",
"posttest": "node set-webpack-version.js \"^4.44.2\" && npm install",
"security": "npm audit --audit-level=moderate",
"test": "npm run test:v4",
"test": "npm run test:v4 && npm run test:v5",
"test:v4": "ava",
"test:v5": "npm install webpack@^5.0.0 --no-save && ava"
"test:v5": "node set-webpack-version.js \"^5\" && npm install && ava"
},
"files": [
"lib",
Expand Down
25 changes: 25 additions & 0 deletions set-webpack-version.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/*
* This file sets Webpack version - both devDependencies and peerDependencies.
*
* This is, for some reason, needed with Windows and maybe npm 6. Running
* "npm install webpack -D" AND manually updating the peerDependencies
* (because npm 6 does not do that but npm 7 does) is not enough. For some
* reason why must, by hand, update the package.json file first and then
* run a normal "npm install".
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is insanity, but THIS turned out to be the reason that asset.info.sourceFilename was missing on Windows (and why I could repeat it). I know from the Webpack folks - webpack/webpack#12637 (comment) - that relying on asset.info.sourceFilename IS the correct way to do things.

Basically, for reasons that aren't clear, on Windows only, if you installed Webpack 5 via npm install webpack -D it would not work. Somewhere, internally, some dependencies must have gotten "crossed". But if you manually change the peerDependencies and devDependencies for Webpack and then run npm install, things work fine. This... almost killed me. But it turns out that this was an edge-case npm + Windows bug of some sort... or possibly some edge case since the package.json file is mixed with libraries that have Webpack 4 vs Webpack 5 as their peer dependency.

Copy link
Owner

Choose a reason for hiding this comment

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

This is fascinating. What we need to know before we merge is why this was an issue only now. We've been testing webpack v4 and webpack v5 separately with the windows workflows without issue up to this point. So what's the missing bit of nuance as to why it's an issue now?

Copy link
Owner

Choose a reason for hiding this comment

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

@weaverryan ping on this comment

const fs = require('fs');
const path = require('path');

const version = process.argv[2];
if (!version) {
console.log('Please pass the webpack version - "^4" or "^5" - as an argument.');

process.exit(1);
}

const packageData = JSON.parse(fs.readFileSync(path.join(__dirname, 'package.json')));
packageData.peerDependencies.webpack = version;

packageData.devDependencies.webpack = version;

fs.writeFileSync(path.join(__dirname, 'package.json'), JSON.stringify(packageData, null, 2));
1 change: 1 addition & 0 deletions test/fixtures/import_image.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
import '../../assets/manifest.svg';
8 changes: 4 additions & 4 deletions test/integration/location.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,12 @@ test('output to the correct location', async (t) => {
filename: '[name].js',
path: outputPath
},
plugins: [new WebpackManifestPlugin({ fileName: 'webpack.manifest.js' })]
plugins: [new WebpackManifestPlugin({ fileName: 'webpack.manifest.json' })]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you end the file in .js, the TerserPlugin tries to parse these files and explodes. I'm not sure why this is different between v4 and v5 (it's only a problem on v5), but it makes sense: the manifest contents are not actually a valid JavaScript file ({ "foo": "bar" } is not valid JavaScript - you'd need to have a const = in front or something equal).

And so, I think this is valid to change. Users are able to control the filename (the point of the test), but in Webpack 5, they cannot name it .js.

Copy link
Owner

Choose a reason for hiding this comment

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

Is that something we should document in the README?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. I would say, let's wait to see if we have any questions about it. The only reason I'm hesitating is that the JSON was never a valid JavaScript file... so I'm trying to think of what the use-case ever would have been (and so, is anyone out there actually doing that?).

};

await compile(config, {}, t);

const manifestPath = join(outputPath, 'webpack.manifest.js');
const manifestPath = join(outputPath, 'webpack.manifest.json');
const result = readJson(manifestPath);

t.deepEqual(result, { 'main.js': 'main.js' });
Expand All @@ -41,11 +41,11 @@ test('output using absolute path', async (t) => {
filename: '[name].js',
path: absOutputPath
},
plugins: [new WebpackManifestPlugin({ fileName: join(absOutputPath, 'webpack.manifest.js') })]
plugins: [new WebpackManifestPlugin({ fileName: join(absOutputPath, 'webpack.manifest.json') })]
};
await compile(config, {}, t);

const manifestPath = join(absOutputPath, 'webpack.manifest.js');
const manifestPath = join(absOutputPath, 'webpack.manifest.json');
const result = readJson(manifestPath);

t.deepEqual(result, { 'main.js': 'main.js' });
Expand Down
45 changes: 37 additions & 8 deletions test/unit/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,15 +75,15 @@ test('works with source maps', async (t) => {
one: '../fixtures/file.js'
},
output: {
filename: '[name].js',
filename: 'build/[name].js',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just making the test more interesting.

path: join(outputPath, 'source-maps')
}
};
const { manifest } = await compile(config, t);

t.deepEqual(manifest, {
'one.js': 'one.js',
'one.js.map': 'one.js.map'
'one.js': 'build/one.js',
'one.js.map': 'build/one.js.map'
});
});

Expand Down Expand Up @@ -158,11 +158,6 @@ test('outputs a manifest of no-js file', async (t) => {
'file.txt': 'file.txt'
};

// Note: I believe this to be another bug in webpack v5 and cannot find a good workaround atm
if (webpack.version.startsWith('5')) {
expected['main.txt'] = 'file.txt';
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turns out, these are now fixed! They are #237


t.truthy(manifest);
t.deepEqual(manifest, expected);
});
Expand All @@ -188,3 +183,37 @@ test('make manifest available to other webpack plugins', async (t) => {
t.pass();
}
});

if (!webpack.version.startsWith('4')) {
test('works with asset modules', async (t) => {
const config = {
context: __dirname,
entry: '../fixtures/import_image.js',
output: {
path: join(outputPath, 'auxiliary-assets'),
assetModuleFilename: `images/[name].[hash:4][ext]`
},
module: {
rules: [
{
test: /\.(svg)/,
type: 'asset/resource'
}
]
}
};

const { manifest } = await compile(config, t);
const expected = {
'main.js': 'main.js',
'images/manifest.svg': `images/manifest.14ca.svg`
};

t.truthy(manifest);
t.deepEqual(Object.keys(expected), ['main.js', 'images/manifest.svg']);
t.deepEqual(manifest['main.js'], 'main.js');
t.regex(manifest['images/manifest.svg'], /images\/manifest\.[a-z|\d]{4}\.svg/);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doing this so I could do a fuzzy check on the hash. The [fullhash] thing does not work for assetModuleFilename, so I needed to use a real hash.

});
} else {
test.skip('works with asset modules', () => {});
}
4 changes: 2 additions & 2 deletions test/unit/manifest-location.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ test('relative path', async (t) => {
};

const { manifest } = await compile(config, t, {
fileName: 'webpack.manifest.js'
fileName: 'webpack.manifest.json'
});

t.deepEqual(manifest, { 'main.js': 'main.js' });
Expand All @@ -31,7 +31,7 @@ test('absolute path', async (t) => {
};

const { manifest } = await compile(config, t, {
fileName: join(outputPath, 'absolute/webpack.manifest.js')
fileName: join(outputPath, 'absolute/webpack.manifest.json')
});

t.deepEqual(manifest, { 'main.js': 'main.js' });
Expand Down
6 changes: 0 additions & 6 deletions test/unit/paths.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ const { join } = require('path');

const test = require('ava');
const del = require('del');
const webpack = require('webpack');

const { compile, hashLiteral } = require('../helpers/unit');

Expand Down Expand Up @@ -200,11 +199,6 @@ test('ensures the manifest is mapping paths to names', async (t) => {
'file.txt': 'outputfile.txt'
};

// Note: I believe this to be another bug in webpack v5 and cannot find a good workaround atm
if (webpack.version.startsWith('5')) {
expected['main.txt'] = 'outputfile.txt';
}

t.truthy(manifest);
t.deepEqual(manifest, expected);
});
Expand Down