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.

Sign up for GitHub

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

Jump to bottom

Support importWorkboxFrom: 'local' in the InjectManifest webpack plugin #1290

Merged
merged 6 commits into from
Feb 13, 2018
Merged

Support importWorkboxFrom: 'local' in the InjectManifest webpack plugin #1290

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
aae6b5a
Support for importWorkboxFrom: 'local' in the InjectManifest plugin
jeffposnick Feb 9, 2018
782a25a
Linting
jeffposnick Feb 9, 2018
fc12572
Updated getManifest() test.
jeffposnick Feb 9, 2018
4a85cf2
Linting, again
jeffposnick Feb 9, 2018
3183d66
Added a publicPath test.
jeffposnick Feb 10, 2018
1302a84
Merge remote-tracking branch 'origin/v3' into inject-manifest-local
jeffposnick Feb 10, 2018
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
2 changes: 1 addition & 1 deletion packages/workbox-build/src/entry-points/options/get-manifest-schema.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,5 @@ const baseSchema = require('./base-schema');

// Define some additional constraints.
module.exports = baseSchema.keys({
globDirectory: joi.string().required(),
globDirectory: joi.string(),
});
20 changes: 14 additions & 6 deletions packages/workbox-webpack-plugin/src/inject-manifest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,16 @@ class InjectManifest {
* @private
*/
async handleEmit(compilation, readFile) {
if (this.config.importWorkboxFrom === 'local') {
throw new Error(`importWorkboxFrom can not be set to 'local' when using` +
` InjectManifest. Please use 'cdn' or a chunk name instead.`);
}

const workboxSWImports = await getWorkboxSWImports(
compilation, this.config);

// this.config.modulePathPrefix may or may not have been set by
// getWorkboxSWImports(), depending on the other config options. If it was
// set, we need to pull it out and make use of it later, as it can't be
// used by the underlying workbox-build getManifest() method.
const modulePathPrefix = this.config.modulePathPrefix;
delete this.config.modulePathPrefix;

let entries = getManifestEntriesFromCompilation(compilation, this.config);
const importScriptsArray = [].concat(this.config.importScripts);

Expand Down Expand Up @@ -113,8 +116,13 @@ class InjectManifest {
.map(JSON.stringify)
.join(', ');

const postInjectionSWString = `importScripts(${importScriptsString});
const setConfigString = modulePathPrefix
? `workbox.setConfig({modulePathPrefix: ` +
`${JSON.stringify(modulePathPrefix)}});`
Copy link

Choose a reason for hiding this comment

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

Do you think JSON.stringify is necessary?

Copy link
Contributor Author

@jeffposnick jeffposnick Feb 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It's safer than wrapping the value in either "..." or '...', just on the off chance modulePathPrefix contains any quote characters. (Not that it should, but still.)

JSON.stringify() is guaranteed to return a properly escaped string.

: '';

const postInjectionSWString = `importScripts(${importScriptsString});
${setConfigString}
${originalSWString}
`;

Expand Down
21 changes: 2 additions & 19 deletions test/workbox-build/node/entry-points/get-manifest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ describe(`[workbox-build] entry-points/get-manifest.js (End to End)`, function()
const BASE_OPTIONS = {
globDirectory: SRC_DIR,
};
const REQUIRED_PARAMS = ['globDirectory'];
const SUPPORTED_PARAMS = [
'dontCacheBustUrlsMatching',
'globDirectory',
'globFollow',
'globIgnores',
'globPatterns',
Expand All @@ -21,7 +21,7 @@ describe(`[workbox-build] entry-points/get-manifest.js (End to End)`, function()
'maximumFileSizeToCacheInBytes',
'modifyUrlPrefix',
'templatedUrls',
].concat(REQUIRED_PARAMS);
];
const UNSUPPORTED_PARAMS = [
'cacheId',
'clientsClaim',
Expand All @@ -38,23 +38,6 @@ describe(`[workbox-build] entry-points/get-manifest.js (End to End)`, function()
'swDest',
];

describe('[workbox-build] required parameters', function() {
for (const requiredParam of REQUIRED_PARAMS) {
it(`should reject with a ValidationError when '${requiredParam}' is missing`, async function() {
const options = Object.assign({}, BASE_OPTIONS);
delete options[requiredParam];

try {
await getManifest(options);
throw new Error('Unexpected success.');
} catch (error) {
expect(error.name).to.eql('ValidationError');
expect(error.details[0].context.key).to.eql(requiredParam);
}
});
}
});

describe('[workbox-build] unsupported parameters', function() {
for (const unsupportedParam of UNSUPPORTED_PARAMS) {
it(`should reject with a ValidationError when '${unsupportedParam}' is present`, async function() {
Expand Down
10 changes: 6 additions & 4 deletions test/workbox-webpack-plugin/node/generate-sw.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,13 @@ describe(`[workbox-webpack-plugin] GenerateSW (End to End)`, function() {
const compiler = webpack(config);
compiler.run((webpackError) => {
if (webpackError) {
expect(webpackError.message.includes('importWorkboxFrom'));
done();
if (webpackError.message.includes('importWorkboxFrom')) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, in the course of writing this PR, I discovered that this test pattern wasn't actually working as it was initially implemented: it should have been expect(webpackError.message.includes('importWorkboxFrom')).to.be.true, although that would then cause an unhandled promise rejection rather than a clean error to report back to mocha.

I took the opportunity to clean up this test along with the corresponding test in the inject-manifest.js suite so that it properly detects the failure and reports it using the done() callback.

done();
} else {
done(new Error(`An unexpected error was thrown: ${webpackError.message}`));
}
} else {
done('Unexpected success.');
done(new Error('Unexpected success.'));
}
});
});
Expand Down Expand Up @@ -304,7 +307,6 @@ describe(`[workbox-webpack-plugin] GenerateSW (End to End)`, function() {
const basenames = libraryFiles.map((file) => path.basename(file));
expect(basenames).to.eql(ALL_WORKBOX_FILES);


// The correct importScripts path should use the versioned name of the
// parent workbox libraries directory. We don't know that version ahead
// of time, so we ensure that there's a match based on what actually
Expand Down
161 changes: 126 additions & 35 deletions test/workbox-webpack-plugin/node/inject-manifest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ const GenerateAssetPlugin = require('generate-asset-webpack-plugin');
const HtmlWebpackPlugin = require('html-webpack-plugin');
const expect = require('chai').expect;
const fse = require('fs-extra');
const glob = require('glob');
const path = require('path');
const tempy = require('tempy');
const vm = require('vm');
Expand All @@ -17,6 +18,47 @@ describe(`[workbox-webpack-plugin] InjectManifest (End to End)`, function() {
const WORKBOX_SW_FILE_NAME = getModuleUrl('workbox-sw');
const SRC_DIR = path.join(__dirname, '..', 'static', 'example-project-1');
const SW_SRC = path.join(__dirname, '..', 'static', 'sw-src.js');
const WORKBOX_DIRECTORY_PREFIX = 'workbox-';
const ALL_WORKBOX_FILES = [
'workbox-background-sync.dev.js',
'workbox-background-sync.dev.js.map',
'workbox-background-sync.prod.js',
'workbox-background-sync.prod.js.map',
'workbox-broadcast-cache-update.dev.js',
'workbox-broadcast-cache-update.dev.js.map',
'workbox-broadcast-cache-update.prod.js',
'workbox-broadcast-cache-update.prod.js.map',
'workbox-cache-expiration.dev.js',
'workbox-cache-expiration.dev.js.map',
'workbox-cache-expiration.prod.js',
'workbox-cache-expiration.prod.js.map',
'workbox-cacheable-response.dev.js',
'workbox-cacheable-response.dev.js.map',
'workbox-cacheable-response.prod.js',
'workbox-cacheable-response.prod.js.map',
'workbox-core.dev.js',
'workbox-core.dev.js.map',
'workbox-core.prod.js',
'workbox-core.prod.js.map',
'workbox-google-analytics.dev.js',
'workbox-google-analytics.dev.js.map',
'workbox-google-analytics.prod.js',
'workbox-google-analytics.prod.js.map',
'workbox-precaching.dev.js',
'workbox-precaching.dev.js.map',
'workbox-precaching.prod.js',
'workbox-precaching.prod.js.map',
'workbox-routing.dev.js',
'workbox-routing.dev.js.map',
'workbox-routing.prod.js',
'workbox-routing.prod.js.map',
'workbox-strategies.dev.js',
'workbox-strategies.dev.js.map',
'workbox-strategies.prod.js',
'workbox-strategies.prod.js.map',
'workbox-sw.js',
'workbox-sw.js.map',
];

describe(`[workbox-webpack-plugin] runtime errors`, function() {
it(`should throw when swSrc is not set`, function() {
Expand Down Expand Up @@ -44,39 +86,13 @@ describe(`[workbox-webpack-plugin] InjectManifest (End to End)`, function() {
const compiler = webpack(config);
compiler.run((webpackError) => {
if (webpackError) {
expect(webpackError.message.includes('importWorkboxFrom'));
done();
} else {
done('Unexpected success.');
}
});
});

it(`should throw when importWorkboxFrom is set to 'local'`, function(done) {
const outputDir = tempy.directory();
const config = {
entry: {
entry1: path.join(SRC_DIR, WEBPACK_ENTRY_FILENAME),
},
output: {
filename: '[name]-[chunkhash].js',
path: outputDir,
},
plugins: [
new InjectManifest({
importWorkboxFrom: 'local',
swSrc: SW_SRC,
}),
],
};

const compiler = webpack(config);
compiler.run((webpackError) => {
if (webpackError) {
expect(webpackError.message.includes('importWorkboxFrom'));
done();
if (webpackError.message.includes('importWorkboxFrom')) {
done();
} else {
done(new Error(`An unexpected error was thrown: ${webpackError.message}`));
}
} else {
done('Unexpected success.');
done(new Error('Unexpected success.'));
}
});
});
Expand Down Expand Up @@ -240,12 +256,87 @@ describe(`[workbox-webpack-plugin] InjectManifest (End to End)`, function() {

const swFile = path.join(outputDir, path.basename(SW_SRC));
try {
// First, validate that the generated service-worker.js meets some basic assumptions.
await validateServiceWorkerRuntime({swFile, expectedMethodCalls: {
importScripts: [[
FILE_MANIFEST_NAME,
workboxEntryName,
]],
suppressWarnings: [[]],
precacheAndRoute: [[[], {}]],
}});

// Next, test the generated manifest to ensure that it contains
// exactly the entries that we expect.
const manifestFileContents = await fse.readFile(path.join(outputDir, FILE_MANIFEST_NAME), 'utf-8');
const context = {self: {}};
vm.runInNewContext(manifestFileContents, context);

const expectedEntries = [{
url: 'entry2-0c3c00f8cd0d3271089c.js',
}, {
url: 'entry1-3865b3908d1988da1758.js',
}];
expect(context.self.__precacheManifest).to.eql(expectedEntries);

done();
} catch (error) {
done(error);
}
});
});

it(`should support setting importWorkboxFrom to 'local'`, function(done) {
Copy link

Choose a reason for hiding this comment

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

It'd be cool to include a test where Webpack publicPath is specified

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.

const FILE_MANIFEST_NAME = 'precache-manifest.b6f6b1b151c4f027ee1e1aa3061eeaf7.js';
const outputDir = tempy.directory();
const config = {
entry: {
entry1: path.join(SRC_DIR, WEBPACK_ENTRY_FILENAME),
entry2: path.join(SRC_DIR, WEBPACK_ENTRY_FILENAME),
},
output: {
filename: '[name]-[chunkhash].js',
path: outputDir,
},
plugins: [
new InjectManifest({
importWorkboxFrom: 'local',
swSrc: SW_SRC,
}),
],
};

const compiler = webpack(config);
compiler.run(async (webpackError) => {
if (webpackError) {
return done(webpackError);
}

const swFile = path.join(outputDir, path.basename(SW_SRC));
try {
// Validate the copied library files.
const libraryFiles = glob.sync(`${WORKBOX_DIRECTORY_PREFIX}*/*.js*`,
{cwd: outputDir});

const modulePathPrefix = path.dirname(libraryFiles[0]);

const basenames = libraryFiles.map((file) => path.basename(file));
expect(basenames).to.eql(ALL_WORKBOX_FILES);

// The correct importScripts path should use the versioned name of the
// parent workbox libraries directory. We don't know that version ahead
// of time, so we ensure that there's a match based on what actually
// got copied over.
const workboxSWImport = libraryFiles.filter(
(file) => file.endsWith('workbox-sw.js'))[0];

// First, validate that the generated service-worker.js meets some basic assumptions.
await validateServiceWorkerRuntime({swFile, expectedMethodCalls: {
importScripts: [[
FILE_MANIFEST_NAME,
workboxEntryName,
workboxSWImport,
]],
setConfig: [[{modulePathPrefix}]],
suppressWarnings: [[]],
precacheAndRoute: [[[], {}]],
}});
Expand All @@ -257,9 +348,9 @@ describe(`[workbox-webpack-plugin] InjectManifest (End to End)`, function() {
vm.runInNewContext(manifestFileContents, context);

const expectedEntries = [{
url: 'entry2-0c3c00f8cd0d3271089c.js',
url: 'entry2-17c2a1b5c94290899539.js',
}, {
url: 'entry1-3865b3908d1988da1758.js',
url: 'entry1-d7f4e7088b64a9896b23.js',
}];
expect(context.self.__precacheManifest).to.eql(expectedEntries);

Expand Down