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

fix(@angular-devkit/build-angular): webpack5 deprecation of module property in Dependency #18884

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -111,7 +111,7 @@ export function getBrowserConfig(wco: WebpackConfigOptions): webpack.Configurati
},
plugins: [
new CommonJsUsageWarnPlugin({
allowedDepedencies: allowedCommonJsDependencies,
allowedDependencies: allowedCommonJsDependencies,
}),
...extraPlugins,
],
Expand Down
@@ -1,4 +1,3 @@

/**
* @license
* Copyright Google Inc. All Rights Reserved.
Expand All @@ -10,6 +9,7 @@
import { isAbsolute } from 'path';
import { Compiler, compilation } from 'webpack';
import { addWarning } from '../../utils/webpack-diagnostics';
import { isWebpackFiveOrHigher } from '../../utils/webpack-version';

// Webpack doesn't export these so the deep imports can potentially break.
const CommonJsRequireDependency = require('webpack/lib/dependencies/CommonJsRequireDependency');
Expand All @@ -28,31 +28,31 @@ interface WebpackModule extends compilation.Module {

export interface CommonJsUsageWarnPluginOptions {
/** A list of CommonJS packages that are allowed to be used without a warning. */
allowedDepedencies?: string[];
allowedDependencies?: string[];
}

export class CommonJsUsageWarnPlugin {
private shownWarnings = new Set<string>();

// Allow the below depedency for HMR
// Allow the below dependency for HMR
// tslint:disable-next-line: max-line-length
// https://github.com/angular/angular-cli/blob/1e258317b1f6ec1e957ee3559cc3b28ba602f3ba/packages/angular_devkit/build_angular/src/dev-server/index.ts#L605-L638
private allowedDepedencies = new Set<string>(['webpack/hot/dev-server']);
private allowedDependencies = new Set<string>(['webpack/hot/dev-server']);

constructor(private options: CommonJsUsageWarnPluginOptions = {}) {
this.options.allowedDepedencies?.forEach(d => this.allowedDepedencies.add(d));
this.options.allowedDependencies?.forEach(d => this.allowedDependencies.add(d));
}

apply(compiler: Compiler) {
compiler.hooks.compilation.tap('CommonJsUsageWarnPlugin', compilation => {
compilation.hooks.finishModules.tap('CommonJsUsageWarnPlugin', modules => {
for (const { dependencies, rawRequest, issuer } of modules as unknown as WebpackModule[]) {
for (const {dependencies, rawRequest, issuer} of modules as unknown as WebpackModule[]) {
if (
!rawRequest ||
rawRequest.startsWith('.') ||
isAbsolute(rawRequest) ||
this.allowedDepedencies.has(rawRequest) ||
this.allowedDepedencies.has(this.rawRequestToPackageName(rawRequest)) ||
this.allowedDependencies.has(rawRequest) ||
this.allowedDependencies.has(this.rawRequestToPackageName(rawRequest)) ||
rawRequest.startsWith('@angular/common/locales/')
) {
/**
Expand All @@ -64,13 +64,13 @@ export class CommonJsUsageWarnPlugin {
continue;
}

if (this.hasCommonJsDependencies(dependencies)) {
if (this.hasCommonJsDependencies(compilation, dependencies)) {
// Dependency is CommonsJS or AMD.

// Check if it's parent issuer is also a CommonJS dependency.
// In case it is skip as an warning will be show for the parent CommonJS dependency.
const parentDependencies = issuer?.issuer?.dependencies;
if (parentDependencies && this.hasCommonJsDependencies(parentDependencies, true)) {
if (parentDependencies && this.hasCommonJsDependencies(compilation, parentDependencies, true)) {
continue;
}

Expand Down Expand Up @@ -100,14 +100,20 @@ export class CommonJsUsageWarnPlugin {
});
}

private hasCommonJsDependencies(dependencies: WebpackModule[], checkParentModules = false): boolean {
private hasCommonJsDependencies(
compilation: compilation.Compilation,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe just provide the moduleGraph instead of the entire compilation object

Copy link
Contributor Author

@valorkin valorkin Sep 25, 2020

Choose a reason for hiding this comment

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

moduleGraph doesn't exist in wepback@4 as far as I know

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to expand my thought a little:

  • if I provide an entire compilation object I can hide and inline types for webpack@5
  • if I will try to provide only moduleGraph, invocation line will contain as unknown as ModuleGraph, 3 times in this file
    So, I though 1st option is more clean

Copy link
Collaborator

Choose a reason for hiding this comment

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

SGTM

dependencies: WebpackModule[],
checkParentModules = false): boolean {
for (const dep of dependencies) {
if (dep instanceof CommonJsRequireDependency || dep instanceof AMDDefineDependency) {
return true;
}

if (checkParentModules && dep.module && this.hasCommonJsDependencies(dep.module.dependencies)) {
return true;
if (checkParentModules) {
const module = getWebpackModule(compilation, dep);
if (module && this.hasCommonJsDependencies(compilation, module.dependencies)) {
return true;
}
}
}

Expand All @@ -121,5 +127,13 @@ export class CommonJsUsageWarnPlugin {
// Non-scoped request ex: lodash/isEmpty -> lodash
: rawRequest.split('/', 1)[0];
}
}

function getWebpackModule(compilation: compilation.Compilation, dependency: WebpackModule): WebpackModule | null {
if (!isWebpackFiveOrHigher()) {
return dependency.module;
}

return (compilation as unknown as { moduleGraph: { getModule(dependency: WebpackModule): WebpackModule; }})
.moduleGraph.getModule(dependency);
}