Skip to content

Commit

Permalink
fix(@angular-devkit/build-angular): match allowed dependencies agains…
Browse files Browse the repository at this point in the history
…t the package name

With this change we add the functionality to also match an allowed dependency against a package name. The package name is retrieved from the rawRequest.

Previously, users needed to add the request path which in some case might be a deep import. Ex: `zone.js/dist/zone-error`. With this change adding the package name example `zone.js` will suffice.

Closes: #18058
  • Loading branch information
alan-agius4 authored and filipesilva committed Jul 1, 2020
1 parent f22efa1 commit d74b7c9
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,10 @@ export class CommonJsUsageWarnPlugin {
// Allow the below depedency 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 = [
'webpack/hot/dev-server',
];
private allowedDepedencies = new Set<string>(['webpack/hot/dev-server']);

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

apply(compiler: Compiler) {
Expand All @@ -57,7 +53,10 @@ export class CommonJsUsageWarnPlugin {
continue;
}

if (this.allowedDepedencies?.includes(rawRequest)) {
if (
this.allowedDepedencies.has(rawRequest) ||
this.allowedDepedencies.has(this.rawRequestToPackageName(rawRequest))
) {
// Skip as this module is allowed even if it's a CommonJS.
continue;
}
Expand All @@ -67,7 +66,8 @@ export class CommonJsUsageWarnPlugin {

// 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.
if (this.hasCommonJsDependencies(issuer?.issuer?.dependencies ?? [])) {
const parentDependencies = issuer?.issuer?.dependencies;
if (parentDependencies && this.hasCommonJsDependencies(parentDependencies)) {
continue;
}

Expand Down Expand Up @@ -100,4 +100,12 @@ export class CommonJsUsageWarnPlugin {
return dependencies.some(d => d instanceof CommonJsRequireDependency || d instanceof AMDDefineDependency);
}

private rawRequestToPackageName(rawRequest: string): string {
return rawRequest.startsWith('@')
// Scoped request ex: @angular/common/locale/en -> @angular/common
? rawRequest.split('/', 2).join('/')
// Non-scoped request ex: lodash/isEmpty -> lodash
: rawRequest.split('/', 1)[0];
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,10 @@ describe('Browser Builder commonjs warning', () => {
architect = (await createArchitect(host.root())).architect;

// Add a Common JS dependency
host.appendToFile('src/app/app.component.ts', `import 'bootstrap';`);
host.appendToFile('src/app/app.component.ts', `
import 'bootstrap';
import 'zone.js/dist/zone-error';
`);

// Create logger
logger = new logging.Logger('');
Expand All @@ -45,6 +48,7 @@ describe('Browser Builder commonjs warning', () => {
const overrides = {
allowedCommonJsDependencies: [
'bootstrap',
'zone.js',
],
};

Expand Down

0 comments on commit d74b7c9

Please sign in to comment.