From b1fa1bf0d599ab55600748c753062169f8ecc926 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Tue, 6 Jul 2021 15:29:19 +0200 Subject: [PATCH] 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 --- BUILD.bazel | 6 +++--- .../ng_rollup_bundle/ng_rollup_bundle.bzl | 4 ++++ .../ng_rollup_bundle/rollup.config-tmpl.js | 14 ++++++++++++++ karma-js.conf.js | 4 ++-- modules/benchmarks/src/tree/ng1/BUILD.bazel | 2 +- modules/playground/src/upgrade/BUILD.bazel | 2 +- package.json | 4 ++-- packages/examples/upgrade/upgrade_example.bzl | 2 +- packages/language-service/bundles/BUILD.bazel | 2 ++ .../src/common/test/helpers/common_test_helpers.ts | 2 +- renovate.json | 4 ++-- yarn.lock | 12 ++++++------ 12 files changed, 39 insertions(+), 19 deletions(-) diff --git a/BUILD.bazel b/BUILD.bazel index a02b4906e2..9e2e0ae584 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -36,18 +36,18 @@ filegroup( srcs = [ # We also declare the unminified AngularJS files since these can be used for # local debugging (e.g. see: packages/upgrade/test/common/test_helpers.ts) - "@npm//:node_modules/angular/angular.js", - "@npm//:node_modules/angular/angular.min.js", "@npm//:node_modules/angular-1.5/angular.js", "@npm//:node_modules/angular-1.5/angular.min.js", "@npm//:node_modules/angular-1.6/angular.js", "@npm//:node_modules/angular-1.6/angular.min.js", "@npm//:node_modules/angular-1.7/angular.js", "@npm//:node_modules/angular-1.7/angular.min.js", - "@npm//:node_modules/angular-mocks/angular-mocks.js", "@npm//:node_modules/angular-mocks-1.5/angular-mocks.js", "@npm//:node_modules/angular-mocks-1.6/angular-mocks.js", "@npm//:node_modules/angular-mocks-1.7/angular-mocks.js", + "@npm//:node_modules/angular-mocks-1.8/angular-mocks.js", + "@npm//:node_modules/angular-1.8/angular.js", + "@npm//:node_modules/angular-1.8/angular.min.js", ], ) diff --git a/dev-infra/benchmark/ng_rollup_bundle/ng_rollup_bundle.bzl b/dev-infra/benchmark/ng_rollup_bundle/ng_rollup_bundle.bzl index 2684c219aa..166fb1bb21 100644 --- a/dev-infra/benchmark/ng_rollup_bundle/ng_rollup_bundle.bzl +++ b/dev-infra/benchmark/ng_rollup_bundle/ng_rollup_bundle.bzl @@ -71,6 +71,10 @@ def ng_rollup_bundle( silent = True, format = format, sourcemap = "true", + # TODO(devversion): consider removing when View Engine is removed. View Engine + # uses Bazel manifest path imports in generated factory files. + # e.g. `import "/<..>/some_file";` + link_workspace_root = True, **kwargs ) diff --git a/dev-infra/benchmark/ng_rollup_bundle/rollup.config-tmpl.js b/dev-infra/benchmark/ng_rollup_bundle/rollup.config-tmpl.js index e480962cc9..ab45b6fe6b 100644 --- a/dev-infra/benchmark/ng_rollup_bundle/rollup.config-tmpl.js +++ b/dev-infra/benchmark/ng_rollup_bundle/rollup.config-tmpl.js @@ -49,6 +49,7 @@ const plugins = [ // https://github.com/angular/angular-cli/blob/1a1ceb609b9a87c4021cce3a6f0fc6d167cd09d2/packages/ngtools/webpack/src/angular_compiler_plugin.ts#L918-L920 mainFields: ivyEnabled ? ['module_ivy_ngcc', 'main_ivy_ngcc', 'module', 'main'] : ['module', 'main'], + preferBuiltins: true, }), commonjs({ignoreGlobal: true}), sourcemaps(), @@ -62,6 +63,7 @@ if (useBuildOptimizer) { module.exports = { plugins, + onwarn: customWarningHandler, external: [TMPL_external], output: { globals: {TMPL_globals}, @@ -69,6 +71,18 @@ module.exports = { } }; +/** Custom warning handler for Rollup. */ +function customWarningHandler(warning, defaultHandler) { + // If rollup is unable to resolve an import, we want to throw an error + // instead of silently treating the import as external dependency. + // https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency + if (warning.code === 'UNRESOLVED_IMPORT') { + throw Error(`Unresolved import: ${warning.message}`); + } + + defaultHandler(warning); +} + /** Extracts the top-level bundle banner if specified. */ function extractBannerIfConfigured() { if (!bannerFile) { diff --git a/karma-js.conf.js b/karma-js.conf.js index 2e122a56a4..956b2860f8 100644 --- a/karma-js.conf.js +++ b/karma-js.conf.js @@ -36,8 +36,8 @@ module.exports = function(config) { {pattern: 'node_modules/angular-mocks-1.6/angular-mocks.js', included: false, watched: false}, {pattern: 'node_modules/angular-1.7/angular?(.min).js', included: false, watched: false}, {pattern: 'node_modules/angular-mocks-1.7/angular-mocks.js', included: false, watched: false}, - {pattern: 'node_modules/angular/angular?(.min).js', included: false, watched: false}, - {pattern: 'node_modules/angular-mocks/angular-mocks.js', included: false, watched: false}, + {pattern: 'node_modules/angular-1.8/angular?(.min).js', included: false, watched: false}, + {pattern: 'node_modules/angular-mocks-1.8/angular-mocks.js', included: false, watched: false}, 'node_modules/core-js-bundle/index.js', 'node_modules/jasmine-ajax/lib/mock-ajax.js', diff --git a/modules/benchmarks/src/tree/ng1/BUILD.bazel b/modules/benchmarks/src/tree/ng1/BUILD.bazel index 7656053b08..677f511ee0 100644 --- a/modules/benchmarks/src/tree/ng1/BUILD.bazel +++ b/modules/benchmarks/src/tree/ng1/BUILD.bazel @@ -16,7 +16,7 @@ ts_library( ts_devserver( name = "devserver", - bootstrap = ["@npm//:node_modules/angular/angular.js"], + bootstrap = ["@npm//:node_modules/angular-1.8/angular.js"], entry_module = "angular/modules/benchmarks/src/tree/ng1/index", port = 4200, static_files = ["index.html"], diff --git a/modules/playground/src/upgrade/BUILD.bazel b/modules/playground/src/upgrade/BUILD.bazel index 93ae3cf13e..fad4b68f10 100644 --- a/modules/playground/src/upgrade/BUILD.bazel +++ b/modules/playground/src/upgrade/BUILD.bazel @@ -20,7 +20,7 @@ ts_devserver( bootstrap = [ "//packages/zone.js/bundles:zone.umd.js", "@npm//:node_modules/reflect-metadata/Reflect.js", - "@npm//:node_modules/angular/angular.js", + "@npm//:node_modules/angular-1.8/angular.js", ], entry_module = "angular/modules/playground/src/upgrade/index", port = 4200, diff --git a/package.json b/package.json index 65b31eac8d..0c63b5871f 100644 --- a/package.json +++ b/package.json @@ -94,14 +94,14 @@ "@types/yaml": "^1.9.7", "@types/yargs": "^16.0.1", "@webcomponents/custom-elements": "^1.1.0", - "angular": "npm:angular@1.8", "angular-1.5": "npm:angular@1.5", "angular-1.6": "npm:angular@1.6", "angular-1.7": "npm:angular@1.7", - "angular-mocks": "npm:angular-mocks@1.8", + "angular-1.8": "npm:angular@1.8", "angular-mocks-1.5": "npm:angular-mocks@1.5", "angular-mocks-1.6": "npm:angular-mocks@1.6", "angular-mocks-1.7": "npm:angular-mocks@1.7", + "angular-mocks-1.8": "npm:angular-mocks@1.8", "base64-js": "1.5.1", "bluebird": "^3.7.2", "brotli": "^1.3.2", diff --git a/packages/examples/upgrade/upgrade_example.bzl b/packages/examples/upgrade/upgrade_example.bzl index d3bca942a7..cd820eee2e 100644 --- a/packages/examples/upgrade/upgrade_example.bzl +++ b/packages/examples/upgrade/upgrade_example.bzl @@ -45,7 +45,7 @@ def create_upgrade_example_targets(name, srcs, e2e_srcs, entry_module, assets = additional_root_paths = ["angular/packages/examples"], bootstrap = [ "//packages/zone.js/bundles:zone.umd.js", - "@npm//:node_modules/angular/angular.js", + "@npm//:node_modules/angular-1.8/angular.js", "@npm//:node_modules/reflect-metadata/Reflect.js", ], static_files = [ diff --git a/packages/language-service/bundles/BUILD.bazel b/packages/language-service/bundles/BUILD.bazel index 08e47e0d38..edb3298c17 100644 --- a/packages/language-service/bundles/BUILD.bazel +++ b/packages/language-service/bundles/BUILD.bazel @@ -6,6 +6,7 @@ ng_rollup_bundle( entry_point = "//packages/language-service:src/ts_plugin.ts", format = "amd", globals = { + "os": "os", "fs": "fs", "path": "path", "typescript": "ts", @@ -26,6 +27,7 @@ ng_rollup_bundle( entry_point = "//packages/language-service/ivy:ts_plugin.ts", format = "amd", globals = { + "os": "os", "fs": "fs", "path": "path", "typescript": "ts", diff --git a/packages/upgrade/src/common/test/helpers/common_test_helpers.ts b/packages/upgrade/src/common/test/helpers/common_test_helpers.ts index 72a02dcd88..f6e5f0ba5f 100644 --- a/packages/upgrade/src/common/test/helpers/common_test_helpers.ts +++ b/packages/upgrade/src/common/test/helpers/common_test_helpers.ts @@ -27,7 +27,7 @@ const ng1Versions = [ }, { label: '1.8', - files: [`angular/${ANGULARJS_FILENAME}`, 'angular-mocks/angular-mocks.js'], + files: [`angular-1.8/${ANGULARJS_FILENAME}`, 'angular-mocks-1.8/angular-mocks.js'], }, ]; diff --git a/renovate.json b/renovate.json index c0b0d7feec..06d7f9a115 100644 --- a/renovate.json +++ b/renovate.json @@ -35,14 +35,14 @@ "@types/babel__traverse", "@types/node", "@types/selenium-webdriver", - "angular", "angular-1.5", "angular-1.6", "angular-1.7", - "angular-mocks", + "angular-1.8", "angular-mocks-1.5", "angular-mocks-1.6", "angular-mocks-1.7", + "angular-mocks-1.8", "puppeteer", "rollup", "selenium-webdriver", diff --git a/yarn.lock b/yarn.lock index da685ee8dc..c31ed3c77a 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2291,6 +2291,11 @@ alphanum-sort@^1.0.2: resolved "https://registry.yarnpkg.com/angular/-/angular-1.7.9.tgz#e52616e8701c17724c3c238cfe4f9446fd570bc4" integrity sha512-5se7ZpcOtu0MBFlzGv5dsM1quQDoDeUTwZrWjGtTNA7O88cD8TEk5IEKCTDa3uECV9XnvKREVUr7du1ACiWGFQ== +"angular-1.8@npm:angular@1.8": + version "1.8.2" + resolved "https://registry.yarnpkg.com/angular/-/angular-1.8.2.tgz#5983bbb5a9fa63e213cb7749199e0d352de3a2f1" + integrity sha512-IauMOej2xEe7/7Ennahkbb5qd/HFADiNuLSESz9Q27inmi32zB0lnAsFeLEWcox3Gd1F6YhNd1CP7/9IukJ0Gw== + "angular-mocks-1.5@npm:angular-mocks@1.5": version "1.5.11" resolved "https://registry.yarnpkg.com/angular-mocks/-/angular-mocks-1.5.11.tgz#a0e1dd0ea55fd77ee7a757d75536c5e964c86f81" @@ -2306,16 +2311,11 @@ alphanum-sort@^1.0.2: resolved "https://registry.yarnpkg.com/angular-mocks/-/angular-mocks-1.7.9.tgz#0a3b7e28b9a493b4e3010ed2b0f69a68e9b4f79b" integrity sha512-LQRqqiV3sZ7NTHBnNmLT0bXtE5e81t97+hkJ56oU0k3dqKv1s6F+nBWRlOVzqHWPGFOiPS8ZJVdrS8DFzHyNIA== -"angular-mocks@npm:angular-mocks@1.8": +"angular-mocks-1.8@npm:angular-mocks@1.8": version "1.8.2" resolved "https://registry.yarnpkg.com/angular-mocks/-/angular-mocks-1.8.2.tgz#dc022420b86978cf317a8447c116c0be73a853bf" integrity sha512-I5L3P0l21HPdVsP4A4qWmENt4ePjjbkDFdAzOaM7QiibFySbt14DptPbt2IjeG4vFBr4vSLbhIz8Fk03DISl8Q== -"angular@npm:angular@1.8": - version "1.8.2" - resolved "https://registry.yarnpkg.com/angular/-/angular-1.8.2.tgz#5983bbb5a9fa63e213cb7749199e0d352de3a2f1" - integrity sha512-IauMOej2xEe7/7Ennahkbb5qd/HFADiNuLSESz9Q27inmi32zB0lnAsFeLEWcox3Gd1F6YhNd1CP7/9IukJ0Gw== - ansi-align@^3.0.0: version "3.0.0" resolved "https://registry.yarnpkg.com/ansi-align/-/ansi-align-3.0.0.tgz#b536b371cf687caaef236c18d3e21fe3797467cb"