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

fix(utils): addEntries fix #2723

Closed
wants to merge 18 commits into from
Closed

fix(utils): addEntries fix #2723

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
ae5b765
fix(server): work on fixing addEntries
knagaitsev Aug 28, 2020
a07a914
fix(entry): handle webpack 5 addEntries as plugin
knagaitsev Aug 28, 2020
317617a
chore(server): create prependEntries util
knagaitsev Sep 6, 2020
438f6a4
chore(server): fix how addEntries works for multi compiler
knagaitsev Sep 6, 2020
e7eae8b
test(server): add module federation test
knagaitsev Sep 6, 2020
ee0c95b
test(utils): working on addEntries new tests
knagaitsev Sep 6, 2020
ca8abc6
test(util): finish addEntries tests
knagaitsev Sep 6, 2020
0cb08c8
chore(server): move prependEntries util back where it was
knagaitsev Sep 6, 2020
6859414
test(server): update server test for webpack 5
knagaitsev Sep 6, 2020
833e70f
chore(server): add close callback
knagaitsev Sep 7, 2020
0a15f2f
chore(server): change how close callback works
knagaitsev Sep 7, 2020
1a91e22
test(server): add async start for test server
knagaitsev Sep 7, 2020
82efa72
test(server): remove error logging for transportMode test
knagaitsev Sep 7, 2020
ce3a419
chore(server): change name of close callback and add comment about ta…
knagaitsev Sep 7, 2020
ebd1e60
test(server): add tests for close callback of addEntries
knagaitsev Sep 7, 2020
6de37f2
chore(server): remove close callback make callback in favor of counter
knagaitsev Sep 10, 2020
dd66b7c
chore(deps): downgrade back to webpack@4
knagaitsev Sep 10, 2020
572767e
Merge branch 'v4' into fix/add-entries
evilebottnawi Nov 17, 2020
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
110 changes: 88 additions & 22 deletions lib/utils/addEntries.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,12 @@ const createDomain = require('./createDomain');

/**
* Add entries Method
* @param {?Object} config - Webpack config
* @param {?Object} originalCompiler - Webpack compiler
* @param {?Object} options - Dev-Server options
* @param {?Object} server
* @returns {void}
*/
function addEntries(config, options, server) {
function addEntries(originalCompiler, options, server) {
// we're stubbing the app in this method as it's static and doesn't require
// a server to be supplied. createDomain requires an app with the
// address() signature.
Expand Down Expand Up @@ -53,42 +53,34 @@ function addEntries(config, options, server) {
} else if (options.hot) {
hotEntry = require.resolve('webpack/hot/dev-server');
}

/**
* prependEntry Method
* prependEntries Method
* @param {Entry} originalEntry
* @param {Entry} additionalEntries
* @returns {Entry}
*/
const prependEntry = (originalEntry, additionalEntries) => {
const prependEntries = (originalEntry, additionalEntries) => {
if (typeof originalEntry === 'function') {
return () =>
Promise.resolve(originalEntry()).then((entry) =>
prependEntry(entry, additionalEntries)
prependEntries(entry, additionalEntries)
);
}

if (typeof originalEntry === 'object' && !Array.isArray(originalEntry)) {
/** @type {Object<string,string>} */
const clone = {};
const entryObj = {};

Object.keys(originalEntry).forEach((key) => {
// entry[key] should be a string here
const entryDescription = originalEntry[key];
if (typeof entryDescription === 'object' && entryDescription.import) {
clone[key] = Object.assign({}, entryDescription, {
import: prependEntry(entryDescription.import, additionalEntries),
});
} else {
clone[key] = prependEntry(entryDescription, additionalEntries);
}
entryObj[key] = prependEntries(entryDescription, additionalEntries);
});

return clone;
return entryObj;
}

// in this case, entry is a string or an array.
// in this case, originalEntry is a string or an array.
// make sure that we do not add duplicates.
/** @type {Entry} */
const entriesClone = additionalEntries.slice(0);
[].concat(originalEntry).forEach((newEntry) => {
if (!entriesClone.includes(newEntry)) {
Expand Down Expand Up @@ -120,8 +112,10 @@ function addEntries(config, options, server) {
return defaultValue;
};

// eslint-disable-next-line no-shadow
[].concat(config).forEach((config) => {
const compilers = originalCompiler.compilers || [originalCompiler];

compilers.forEach((compiler) => {
const config = compiler.options;
/** @type {Boolean} */
const webTarget = [
'web',
Expand All @@ -145,8 +139,7 @@ function addEntries(config, options, server) {
additionalEntries.push(hotEntry);
}

config.entry = prependEntry(config.entry || './src', additionalEntries);

// add the HMR plugin to each compiler config if hot is enabled
if (options.hot || options.hot === 'only') {
config.plugins = config.plugins || [];
if (
Expand All @@ -159,6 +152,79 @@ function addEntries(config, options, server) {
config.plugins.push(new webpack.HotModuleReplacementPlugin());
}
}

// if there are no additional entries to be added, nothing
// below is needed
if (!additionalEntries.length) {
return;
}

if (
webpack.version[0] === '5' &&
!compiler.hasWebpackDevServerMakeCallback
) {
// this counter allows the dev server to ensure only the most recent
// make callback is used, preventing duplicate entry injections
if (compiler.webpackDevServerMakeCallbackCount) {
// this invalidates old make callbacks
compiler.webpackDevServerMakeCallbackCount += 1;
} else {
compiler.webpackDevServerMakeCallbackCount = 1;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My idea was to make a counter for callbacks, so that only the most recent callback will be used, fixing the problem I mentioned. Let me know if this seems too hacky.

}
// this is the index of this particular make hook tap, meaning
// this callback should only do something if it is the most recently
// created callback
const makeCallbackIndex = compiler.webpackDevServerMakeCallbackCount;

// EntryPlugin is only available for webpack@5
// @ts-ignore
const EntryPlugin = require('webpack/lib/EntryPlugin'); // eslint-disable-line import/no-unresolved
// for webpack@5, we use a plugin to add global entries
compiler.hooks.make.tapAsync(
{
name: 'webpack-dev-server',
stage: -100,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@evilebottnawi @sokra I've added the tapAsync method here, but the issue I've found now is that something like this where multiple Servers are attached to a compiler no longer works:

const compiler = webpack(config);
const server1 = new Server(compiler, options1);
const server2 = new Server(compiler, options2);

as it results in something like Error: Conflicting entry option static = [object Object] vs [object Object]

This worked before, as there were tests that did this which are now broken. (These tests would usually close the old server before a new one was attached)

Should we go about this by making multiple servers on one compiler discouraged, or maybe a hacky method of disabling the old hook taps like in webpack/tapable#109 after the server is closed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Or I could also try to prevent duplicate entries by looking at the compilation globalEntry object.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added the hacky method for disabling old hook taps, but haven't added tests for it yet

Copy link
Member

Choose a reason for hiding this comment

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

@Loonride I think we should prevent duplicate, in theory we can have property on compiler to achieve this, we use same logic here https://github.com/webpack/webpack-dev-middleware/blob/master/src/utils/setupWriteToDisk.js#L9 (but it only for webpack@4)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@evilebottnawi That is a good idea, do you think I should remove my closeCallback method and replace it with setting something like compiler.hasWebpackDevServerMakeCallback = true?

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 is more clean hack 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@evilebottnawi I agree it is more clean, but one issue I now see is that if the user does:

const compiler = webpack(config);
const server1 = new Server(compiler, options1);
server1.close(() => {
  const server2 = new Server(compiler, options2);
});

If the options2 object results in a new set of entries, that will not be reflected in the compilation, since the old make callback would still be used.

},
(compilation, callback) => {
if (
makeCallbackIndex !== compiler.webpackDevServerMakeCallbackCount
) {
return callback();
}

const promises = additionalEntries.map((additionalEntry) => {
return new Promise((fulfill, reject) => {
const depOptions = {
// eslint-disable-next-line no-undefined
name: undefined, // global entry, added before all entries
};
const dep = EntryPlugin.createDependency(
additionalEntry,
depOptions
);
compilation.addEntry(compiler.context, dep, options, (err) => {
if (err) {
return reject(err);
}
return fulfill();
});
});
});

Promise.all(promises)
.then(() => {
callback();
})
.catch(callback);
}
);
} else {
// this is a hacky method of injecting entries after compiler creation
// that should only be used for webpack@4
const originalEntry = config.entry || './src';
config.entry = prependEntries(originalEntry, additionalEntries);
compiler.hooks.entryOption.call(config.context, config.entry);
}
});
}

Expand Down
9 changes: 1 addition & 8 deletions lib/utils/updateCompiler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,19 +18,15 @@ function updateCompiler(compiler, options) {

const compilers = [];
const compilersWithoutHMR = [];
let webpackConfig;
if (compiler.compilers) {
webpackConfig = [];
// eslint-disable-next-line no-shadow
compiler.compilers.forEach((compiler) => {
webpackConfig.push(compiler.options);
compilers.push(compiler);
if (!findHMRPlugin(compiler.options)) {
compilersWithoutHMR.push(compiler);
}
});
} else {
webpackConfig = compiler.options;
compilers.push(compiler);
if (!findHMRPlugin(compiler.options)) {
compilersWithoutHMR.push(compiler);
Expand All @@ -42,12 +38,9 @@ function updateCompiler(compiler, options) {
// the changes we are making to the compiler
// important: this relies on the fact that addEntries now
// prevents duplicate new entries.
addEntries(webpackConfig, options);
addEntries(compiler, options);
// eslint-disable-next-line no-shadow
compilers.forEach((compiler) => {
const config = compiler.options;
compiler.hooks.entryOption.call(config.context, config.entry);

const providePlugin = new webpack.ProvidePlugin({
__webpack_dev_server_client__: getSocketClientPath(options),
});
Expand Down