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

Track Angular polyfills #913

Merged
merged 3 commits into from
Jan 17, 2025
Merged
Show file tree
Hide file tree
Changes from 2 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
6 changes: 2 additions & 4 deletions packages/knip/fixtures/plugins/angular/angular.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,7 @@
"outputPath": "dist/knip-angular-example",
"index": "src/index.html",
"main": "src/main.ts",
"polyfills": [
"zone.js"
],
"polyfills": "src/polyfill.js",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are polyfills potentially external? I.e. the zone.js example might be a package reference, which should then go into toDeferResolve. Knip will try to resolve the specifier and mark the external dependency as (un)used or use the internal path as an entry file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely! They may be external. Just pushed 1a78aae to take that into account. Just out of curiosity, what's the difference between toDeferResolve and toDeferResolveEntry? 🤔

Thanks :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checked also that no other option allows for external imports. At least for the application builder https://github.com/angular/angular-cli/blob/main/packages/angular/build/src/builders/application/schema.json#L78 but from what I can't recall that's the same behaviour for previous builders too

Copy link
Collaborator

Choose a reason for hiding this comment

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

Absolutely! They may be external. Just pushed 1a78aae to take that into account. Just out of curiosity, what's the difference between toDeferResolve and toDeferResolveEntry? 🤔

Thanks :)

The fact that I had to look this up isn't great 😅

  • toDeferResolve: needs, resolving, reported as unresolved if that's the case
  • toDeferResolveEntry: needs resolving, meant as entry file, ignored if file doesn't exist
  • toEntry: doesn't need extra resolving, ignored if file doesn't exist

Will try to come up with better names/docs. Suggestions welcome :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hahah naming is hard! thanks for looking it up 😊 can't think of better ones rn :S

"tsConfig": "tsconfig.app.json",
"inlineStyleLanguage": "scss",
"assets": [
Expand Down Expand Up @@ -103,4 +101,4 @@
}
}
}
}
}
3 changes: 3 additions & 0 deletions packages/knip/fixtures/plugins/angular/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
"watch": "ng build --watch --configuration development",
"test": "ng test"
},
"dependencies": {
"zone.js": "*"
},
"devDependencies": {
"@angular/cli": "*",
"karma": "*",
Expand Down
Empty file.
2 changes: 1 addition & 1 deletion packages/knip/fixtures/plugins/angular2/angular.json
Original file line number Diff line number Diff line change
Expand Up @@ -105,4 +105,4 @@
}
}
}
}
}
3 changes: 3 additions & 0 deletions packages/knip/fixtures/plugins/angular2/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
"scripts": {
"ng": "ng"
},
"dependencies": {
"zone.js": "*"
},
"devDependencies": {
"@angular/cli": "*",
"@angular/ssr": "*",
Expand Down
10 changes: 7 additions & 3 deletions packages/knip/fixtures/plugins/angular3/angular.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,19 @@
"ssr": {
"entry": "src/server.ts"
},
"server": "src/main.server-for-non-prod.ts"
"server": "src/main.server-for-non-prod.ts",
"polyfills": [
"src/polyfill.js"
]
},
"configurations": {
"production": {
"server": "src/main.server.ts",
"scripts": ["src/script.js"]
},
"development": {
"scripts": ["src/script-for-non-prod.js"]
"scripts": ["src/script-for-non-prod.js"],
"polyfills": ["src/polyfill-for-non-prod.js"]
}
}
},
Expand All @@ -43,4 +47,4 @@
}
}
}
}
}
Empty file.
Empty file.
40 changes: 30 additions & 10 deletions packages/knip/src/plugins/angular/index.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { existsSync } from 'node:fs';
import type { IsPluginEnabled, Plugin, ResolveConfig } from '../../types/config.js';
import { type Input, toConfig, toDependency, toEntry, toProductionEntry } from '../../util/input.js';
import { join } from '../../util/path.js';
import { type Input, toConfig, toDeferResolve, toDependency, toEntry, toProductionEntry } from '../../util/input.js';
import { isInternal, join } from '../../util/path.js';
import { hasDependency } from '../../util/plugin.js';
import * as karma from '../karma/helpers.js';
import type {
Expand Down Expand Up @@ -44,24 +45,37 @@ const resolveConfig: ResolveConfig<AngularCLIWorkspaceConfiguration> = async (co
);
const productionEntriesByOption: EntriesByOption =
entriesByOptionByConfig.get(PRODUCTION_CONFIG_NAME) ?? new Map();
const normalizePath = (path: string) => join(cwd, path);
const isBuildTarget = targetName === BUILD_TARGET_NAME;
const maybeExternal = (option: string) => option === 'polyfills';
const toInput = (specifier: string, opts: { isProduction: boolean; maybeExternal: boolean }): Input => {
const normalizedPath = join(cwd, specifier);
// 👇 `isInternal` will report `false` for specifiers not starting with `.`
// However, relative imports are usually specified in `angular.json` without `.` prefix
// Hence checking also that file doesn't exist before considering it external
if (opts.maybeExternal && !isInternal(specifier) && !existsSync(normalizedPath)) {
return toDeferResolve(specifier);
}
return opts.isProduction ? toEntry(normalizedPath) : toProductionEntry(normalizedPath);
davidlj95 marked this conversation as resolved.
Show resolved Hide resolved
};
for (const [configName, entriesByOption] of entriesByOptionByConfig.entries()) {
for (const entries of entriesByOption.values()) {
for (const [option, entries] of entriesByOption.entries()) {
for (const entry of entries) {
inputs.add(
targetName === BUILD_TARGET_NAME && configName === PRODUCTION_CONFIG_NAME
? toProductionEntry(normalizePath(entry))
: toEntry(normalizePath(entry))
toInput(entry, {
isProduction: isBuildTarget && configName === PRODUCTION_CONFIG_NAME,
maybeExternal: maybeExternal(option),
})
);
}
}
}
for (const [option, entries] of defaultEntriesByOption.entries()) {
for (const entry of entries) {
inputs.add(
targetName === BUILD_TARGET_NAME && !productionEntriesByOption.get(option)?.length
? toProductionEntry(normalizePath(entry))
: toEntry(normalizePath(entry))
toInput(entry, {
isProduction: isBuildTarget && !productionEntriesByOption.get(option)?.length,
maybeExternal: maybeExternal(option),
})
);
}
}
Expand Down Expand Up @@ -109,6 +123,12 @@ const entriesByOption = (opts: TargetOptions): EntriesByOption =>
typeof scriptStringOrObject === 'string' ? scriptStringOrObject : scriptStringOrObject.input
)
: [],
polyfills:
'polyfills' in opts && opts.polyfills
? Array.isArray(opts.polyfills)
? opts.polyfills
: [opts.polyfills]
: [],
fileReplacements:
'fileReplacements' in opts && opts.fileReplacements && Array.isArray(opts.fileReplacements)
? (opts.fileReplacements as FileReplacementsBuildOption).map(fileReplacement =>
Expand Down
4 changes: 2 additions & 2 deletions packages/knip/test/plugins/angular.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ test('Find dependencies with the Angular plugin', async () => {
devDependencies: 1,
unlisted: 1,
unresolved: 1,
processed: 3,
total: 3,
processed: 4,
total: 4,
});
});
8 changes: 4 additions & 4 deletions packages/knip/test/plugins/angular3.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ test('Find dependencies with the Angular plugin (non-production)', async () => {
assert.deepEqual(counters, {
...baseCounters,
devDependencies: 1,
processed: 8,
total: 8,
processed: 10,
total: 10,
});
});

Expand All @@ -32,7 +32,7 @@ test('Find dependencies with the Angular plugin (production)', async () => {

assert.deepEqual(counters, {
...baseCounters,
processed: 4,
total: 4,
processed: 5,
total: 5,
});
});
Loading