Skip to content

Commit 24849fc

Browse files
devversionjosephperrott
authored andcommitted
fix(dev-infra): ng_rollup_bundle rule should error if import cannot be resolved (#42760)
Rollup just prints a warning if an import cannot be resolved and ends up being treated as an external dependency. This in combination with the `silent = True` attribute for `rollup_bundle` means that bundles might end up being extremely small without people noticing that it misses actual imports. To improve this situation, the warning is replaced by an error if an import cannot be resolved. This unveiles an issue with the `ng_rollup_bundle` macro from dev-infra where imports in View Engine were not resolved but ended up being treated as external. This did not prevent benchmarks using this macro from working because the ConcatJS devserver had builtin resolution for workspace manifest paths. Though given the new check for no unresolved imports, this will now cause errors within Rollup, and we need to fix the resolution. We can fix the issue by temporarily enabling workspace linking. This does not have any performance downsides. To enable workspace linking (which we might need more often in the future given the linker taking over patched module resolution), we had to rename the `angular` dependency to a more specific one so that the Angular linker could link into `node_modules/angular`. PR Close #42760
1 parent 0bab621 commit 24849fc

File tree

2 files changed

+18
-0
lines changed

2 files changed

+18
-0
lines changed

benchmark/ng_rollup_bundle/ng_rollup_bundle.bzl

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,10 @@ def ng_rollup_bundle(
7171
silent = True,
7272
format = format,
7373
sourcemap = "true",
74+
# TODO(devversion): consider removing when View Engine is removed. View Engine
75+
# uses Bazel manifest path imports in generated factory files.
76+
# e.g. `import "<workspace_root>/<..>/some_file";`
77+
link_workspace_root = True,
7478
**kwargs
7579
)
7680

benchmark/ng_rollup_bundle/rollup.config-tmpl.js

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ const plugins = [
4949
// https://github.com/angular/angular-cli/blob/1a1ceb609b9a87c4021cce3a6f0fc6d167cd09d2/packages/ngtools/webpack/src/angular_compiler_plugin.ts#L918-L920
5050
mainFields: ivyEnabled ? ['module_ivy_ngcc', 'main_ivy_ngcc', 'module', 'main'] :
5151
['module', 'main'],
52+
preferBuiltins: true,
5253
}),
5354
commonjs({ignoreGlobal: true}),
5455
sourcemaps(),
@@ -62,13 +63,26 @@ if (useBuildOptimizer) {
6263

6364
module.exports = {
6465
plugins,
66+
onwarn: customWarningHandler,
6567
external: [TMPL_external],
6668
output: {
6769
globals: {TMPL_globals},
6870
banner: extractBannerIfConfigured(),
6971
}
7072
};
7173

74+
/** Custom warning handler for Rollup. */
75+
function customWarningHandler(warning, defaultHandler) {
76+
// If rollup is unable to resolve an import, we want to throw an error
77+
// instead of silently treating the import as external dependency.
78+
// https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency
79+
if (warning.code === 'UNRESOLVED_IMPORT') {
80+
throw Error(`Unresolved import: ${warning.message}`);
81+
}
82+
83+
defaultHandler(warning);
84+
}
85+
7286
/** Extracts the top-level bundle banner if specified. */
7387
function extractBannerIfConfigured() {
7488
if (!bannerFile) {

0 commit comments

Comments
 (0)