fix(dev-infra): ng_rollup_bundle rule should error if import cannot be resolved (angular#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 angular#42760

Author: devversion

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",
     ],
 )
 

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 "<workspace_root>/<..>/some_file";`
+        link_workspace_root = True,
         **kwargs
     )
 

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,13 +63,26 @@ if (useBuildOptimizer) {
 
 module.exports = {
   plugins,
+  onwarn: customWarningHandler,
   external: [TMPL_external],
   output: {
     globals: {TMPL_globals},
     banner: extractBannerIfConfigured(),
   }
 };
 
+/** 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) {

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',

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"],

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,

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",

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 = [

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",

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'],
   },
 ];