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

fix(compiler-cli): incorrect bundled metadata for static class member call expressions #28762

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions integration/bazel/BUILD.bazel
Expand Up @@ -2,6 +2,7 @@ package(default_visibility = ["//visibility:public"])

exports_files([
"protractor.conf.js",
"angular-metadata.tsconfig.json",
])

# ts_library and ng_module use the `//:tsconfig.json` target
Expand Down
15 changes: 11 additions & 4 deletions integration/bazel/WORKSPACE
Expand Up @@ -56,9 +56,13 @@ node_repositories(
# Install our npm dependencies into @npm
yarn_install(
name = "npm",
# Need a reference to @angular here so that Bazel sets up the
# external repository before calling yarn_install
data = ["@angular//:LICENSE"],
data = [
# Needed because this tsconfig file is used in the "postinstall" script.
"//:angular-metadata.tsconfig.json",
# Need a reference to @angular here so that Bazel sets up the
# external repository before calling yarn_install
"@angular//:LICENSE",
],
package_json = "//src:package.json",
yarn_lock = "//src:yarn.lock",
)
Expand Down Expand Up @@ -94,7 +98,10 @@ load("@io_bazel_rules_sass//sass:sass_repositories.bzl", "sass_repositories")

sass_repositories()

# Setup the angular toolchain
# Setup the angular toolchain. This integration test no longer builds Angular from source,
# but we still need to set up the "angular" workspace since some Bazel rules depend on
# the "ngdeps" repository. This can be fixed if we switched the Angular repository to the
# "npm" repository for the bazel managed dependencies.
load("@angular//:index.bzl", "ng_setup_workspace")

ng_setup_workspace()
21 changes: 21 additions & 0 deletions integration/bazel/angular-metadata.tsconfig.json
@@ -0,0 +1,21 @@
// Workaround for https://github.com/angular/angular/issues/18810
// This file is required because when using the Angular NPM packages and building
// with AOT compilation, NGC needs the "ngsummary.json" files.
{
"compilerOptions": {
"lib": [
"dom",
"es2015"
],
"experimentalDecorators": true,
"types": []
},
"include": [
"node_modules/@angular/**/*"
],
"exclude": [
"node_modules/@angular/bazel/**",
"node_modules/@angular/compiler-cli/**",
"node_modules/@angular/**/testing/**"
]
}
31 changes: 21 additions & 10 deletions integration/bazel/src/BUILD.bazel
Expand Up @@ -10,9 +10,9 @@ ng_module(
srcs = glob(["*.ts"]),
deps = [
"//src/hello-world",
"@angular//packages/common/http",
"@angular//packages/core",
"@angular//packages/platform-browser",
"@npm//@angular/common",
"@npm//@angular/core",
"@npm//@angular/platform-browser",
"@npm//@types",
],
)
Expand All @@ -26,26 +26,37 @@ ts_devserver(
"npm/node_modules/zone.js/dist",
],
entry_module = "bazel_integration_test/src/main",
scripts = [
"@npm//node_modules/@angular/common:bundles/common.umd.js",
"@npm//node_modules/@angular/common:bundles/common-http.umd.js",
"@npm//node_modules/@angular/core:bundles/core.umd.js",
"@npm//node_modules/@angular/platform-browser:bundles/platform-browser.umd.js",
],
serving_path = "/bundle.min.js",
static_files = [
"@npm//node_modules/tslib:tslib.js",
"@npm//node_modules/zone.js:dist/zone.min.js",
"index.html",
],
deps = ["//src"],
deps = [
"//src",
# This will be removed with https://github.com/angular/angular/pull/28720. This is the
# only remaining dependency that we still build from source here.
"@rxjs",
],
)

load("@build_bazel_rules_nodejs//:defs.bzl", "nodejs_binary", "rollup_bundle")

filegroup(
name = "empty_node_modules",
)

rollup_bundle(
name = "bundle",
entry_point = "src/main",
node_modules = ":empty_node_modules",
deps = ["//src"],
deps = [
"//src",
"@npm//@angular/common",
"@npm//@angular/core",
"@npm//@angular/platform-browser",
],
)

# Needed because the prodserver only loads static files that appear under this
Expand Down
25 changes: 19 additions & 6 deletions integration/bazel/src/hello-world/BUILD.bazel
Expand Up @@ -18,7 +18,7 @@ ng_module(
),
assets = [":hello-world-styles"],
deps = [
"@angular//packages/core",
"@npm//@angular/core",
"@npm//@types",
],
)
Expand All @@ -35,18 +35,28 @@ ts_library(
srcs = glob(["*.spec.ts"]),
deps = [
":hello-world",
"@angular//packages/core",
"@angular//packages/core/testing",
"@angular//packages/platform-browser",
"@angular//packages/platform-browser-dynamic/testing",
"@npm//@angular/core",
"@npm//@angular/platform-browser",
"@npm//@angular/platform-browser-dynamic",
"@npm//@types",
"@npm//jasmine",
],
)

ts_web_test_suite(
name = "test",
srcs = ["@npm//node_modules/tslib:tslib.js"],
srcs = [
"@npm//node_modules/@angular/common:bundles/common.umd.js",
"@npm//node_modules/@angular/compiler:bundles/compiler.umd.js",
"@npm//node_modules/@angular/compiler:bundles/compiler-testing.umd.js",
"@npm//node_modules/@angular/core:bundles/core.umd.js",
"@npm//node_modules/@angular/core:bundles/core-testing.umd.js",
"@npm//node_modules/@angular/platform-browser:bundles/platform-browser.umd.js",
"@npm//node_modules/@angular/platform-browser:bundles/platform-browser-testing.umd.js",
"@npm//node_modules/@angular/platform-browser-dynamic:bundles/platform-browser-dynamic.umd.js",
"@npm//node_modules/@angular/platform-browser-dynamic:bundles/platform-browser-dynamic-testing.umd.js",
"@npm//node_modules/tslib:tslib.js",
],
bootstrap = [
"@npm//node_modules/zone.js:dist/zone-testing-bundle.js",
"@npm//node_modules/reflect-metadata:Reflect.js",
Expand All @@ -59,5 +69,8 @@ ts_web_test_suite(
],
deps = [
":test_lib",
# This will be removed with https://github.com/angular/angular/pull/28720. This is the
# only remaining dependency that we still build from source here.
"@rxjs",
],
)
8 changes: 7 additions & 1 deletion integration/bazel/src/package.json
Expand Up @@ -4,6 +4,11 @@
"version": "0.0.0",
"license": "MIT",
"dependencies": {
"@angular/animations": "file:../angular/dist/packages-dist/animations",
"@angular/common": "file:../angular/dist/packages-dist/common",
"@angular/core": "file:../angular/dist/packages-dist/core",
"@angular/platform-browser": "file:../angular/dist/packages-dist/platform-browser",
"@angular/platform-browser-dynamic": "file:../angular/dist/packages-dist/platform-browser-dynamic",
"reflect-metadata": "0.1.12",
"rxjs": "6.3.3",
"tslib": "1.9.3",
Expand All @@ -21,6 +26,7 @@
"typescript": "3.1.1"
},
"scripts": {
"postinstall": "ngc -p ./angular-metadata.tsconfig.json",
"//": "TODO(gregmagolan): figure out how to keep dependencies here up to date with the root package.json"
}
}
}
64 changes: 49 additions & 15 deletions integration/bazel/src/yarn.lock
Expand Up @@ -32,10 +32,10 @@
rxjs "6.3.3"
source-map "0.7.3"

"@angular-devkit/core@7.3.0":
version "7.3.0"
resolved "https://registry.yarnpkg.com/@angular-devkit/core/-/core-7.3.0.tgz#fc272e39b4c307833e9a7db77007418a246f5410"
integrity sha512-b0qtAUpgqLpWY8W6vWRv1aj6bXkZCP1rvywl8i8TbGMY67CWRcy5J3fNAMmjiZS+LJixFlIXYf4iOydglyJMfg==
"@angular-devkit/core@7.3.1":
version "7.3.1"
resolved "https://registry.yarnpkg.com/@angular-devkit/core/-/core-7.3.1.tgz#d92f6545796579cabdcfc29579a2c977f7a96c6c"
integrity sha512-56XDWWfIzOAkEk69lBLgmCYybPUA4yjunhmMlCk7vVdb7gbQUyzNjFD04Uj0GjlejatAQ5F76tRwygD9C+3RXQ==
dependencies:
ajv "6.7.0"
chokidar "2.0.4"
Expand All @@ -51,29 +51,39 @@
"@angular-devkit/core" "7.1.2"
rxjs "6.3.3"

"@angular-devkit/schematics@^7.3.0-rc.0":
version "7.3.0"
resolved "https://registry.yarnpkg.com/@angular-devkit/schematics/-/schematics-7.3.0.tgz#112c1f59ff2778157aff6fb7484a6c132d4156ac"
integrity sha512-glOduymftH0LmJhITWgWUJK8QCDUltgTZ943/OyArIvLXTLL/8zCb+G6xL+3k33EQjwJicgQ3WIjonJmeTK/Ww==
"@angular-devkit/schematics@^7.0.4":
version "7.3.1"
resolved "https://registry.yarnpkg.com/@angular-devkit/schematics/-/schematics-7.3.1.tgz#7dc704005b966ea6c1ee62f380120183bb76eee6"
integrity sha512-cd7usiasfSgw75INz72/VssrLr9tiVRYfo1TEdvr9ww0GuQbuQpB33xbV8W135eAV8+wzQ3Ce8ohaDHibvj6Yg==
dependencies:
"@angular-devkit/core" "7.3.0"
"@angular-devkit/core" "7.3.1"
rxjs "6.3.3"

"@angular/animations@file:../angular/dist/packages-dist/animations":
version "0.0.0"
dependencies:
tslib "^1.9.0"

"@angular/bazel@file:../angular/dist/packages-dist/bazel":
version "8.0.0-beta.2"
version "0.0.0"
dependencies:
"@angular-devkit/architect" "^0.10.6"
"@angular-devkit/core" "^7.0.4"
"@angular-devkit/schematics" "^7.3.0-rc.0"
"@bazel/typescript" "^0.23.2"
"@angular-devkit/schematics" "^7.0.4"
"@bazel/typescript" "^0.21.0"
"@schematics/angular" "^7.0.4"
"@types/node" "6.0.84"
semver "^5.6.0"
shelljs "0.8.2"
tsickle "0.34.0"

"@angular/common@file:../angular/dist/packages-dist/common":
version "0.0.0"
dependencies:
tslib "^1.9.0"

"@angular/compiler-cli@file:../angular/dist/packages-dist/compiler-cli":
version "8.0.0-beta.2"
version "0.0.0"
dependencies:
canonical-path "1.0.0"
chokidar "^1.4.2"
Expand All @@ -88,7 +98,22 @@
yargs "9.0.1"

"@angular/compiler@file:../angular/dist/packages-dist/compiler":
version "8.0.0-beta.2"
version "0.0.0"
dependencies:
tslib "^1.9.0"

"@angular/core@file:../angular/dist/packages-dist/core":
version "0.0.0"
dependencies:
tslib "^1.9.0"

"@angular/platform-browser-dynamic@file:../angular/dist/packages-dist/platform-browser-dynamic":
version "0.0.0"
dependencies:
tslib "^1.9.0"

"@angular/platform-browser@file:../angular/dist/packages-dist/platform-browser":
version "0.0.0"
dependencies:
tslib "^1.9.0"

Expand All @@ -109,7 +134,7 @@
semver "5.6.0"
tmp "0.0.33"

"@bazel/typescript@0.23.2", "@bazel/typescript@^0.23.2":
"@bazel/typescript@0.23.2":
version "0.23.2"
resolved "https://registry.yarnpkg.com/@bazel/typescript/-/typescript-0.23.2.tgz#a3ff199880855259d84216cb41644c1d9a0fad14"
integrity sha512-GrTyDW6Fvp/rgnxZGYampB5/QmDWvxtLEtUyMCPa/QXFR1OVxaMWeHxxuFEcES2UKJegqBDKAA8IzX21x4UbEw==
Expand All @@ -120,6 +145,15 @@
source-map-support "0.5.9"
tsutils "2.27.2"

"@bazel/typescript@^0.21.0":
version "0.21.0"
resolved "https://registry.yarnpkg.com/@bazel/typescript/-/typescript-0.21.0.tgz#41c304f77a42c6a016280d0f4c20e0749c3f4b2a"
integrity sha512-ASXj0RFybmqoa3LwqkTU3gNkX9bY9wL/VDNo5hlp9pynYWl4RMpe9V3m/qDIdtSuLJ+qD+Z3FKT/OcpWQHMlYA==
dependencies:
protobufjs "5.0.3"
source-map-support "0.5.9"
tsutils "2.27.2"

"@schematics/angular@^7.0.4":
version "7.1.2"
resolved "https://registry.yarnpkg.com/@schematics/angular/-/angular-7.1.2.tgz#b3eefbc81d12b0b53816896f6172eb613885826c"
Expand Down
14 changes: 12 additions & 2 deletions packages/compiler-cli/src/metadata/bundler.ts
Expand Up @@ -11,7 +11,7 @@ import * as ts from 'typescript';
import {MetadataCache} from '../transformers/metadata_cache';

import {MetadataCollector} from './collector';
import {ClassMetadata, ConstructorMetadata, FunctionMetadata, METADATA_VERSION, MemberMetadata, MetadataEntry, MetadataError, MetadataImportedSymbolReferenceExpression, MetadataMap, MetadataObject, MetadataSymbolicExpression, MetadataSymbolicReferenceExpression, MetadataValue, MethodMetadata, ModuleExportMetadata, ModuleMetadata, isClassMetadata, isConstructorMetadata, isFunctionMetadata, isInterfaceMetadata, isMetadataError, isMetadataGlobalReferenceExpression, isMetadataImportedSymbolReferenceExpression, isMetadataModuleReferenceExpression, isMetadataSymbolicExpression, isMethodMetadata} from './schema';
import {ClassMetadata, ConstructorMetadata, FunctionMetadata, METADATA_VERSION, MemberMetadata, MetadataEntry, MetadataError, MetadataMap, MetadataObject, MetadataSymbolicExpression, MetadataSymbolicReferenceExpression, MetadataValue, MethodMetadata, ModuleExportMetadata, ModuleMetadata, isClassMetadata, isConstructorMetadata, isFunctionMetadata, isInterfaceMetadata, isMetadataError, isMetadataGlobalReferenceExpression, isMetadataImportedSymbolReferenceExpression, isMetadataModuleReferenceExpression, isMetadataSymbolicCallExpression, isMetadataSymbolicExpression, isMethodMetadata} from './schema';



Expand Down Expand Up @@ -412,7 +412,17 @@ export class MetadataBundler {
let result: StaticsMetadata = {};
for (const key in statics) {
const value = statics[key];
result[key] = isFunctionMetadata(value) ? this.convertFunction(moduleName, value) : value;

if (isFunctionMetadata(value)) {
result[key] = this.convertFunction(moduleName, value);
} else if (isMetadataSymbolicCallExpression(value)) {
// Class members can also contain static members that call a function with module
// references. e.g. "static ngInjectableDef = defineInjectable(..)". We also need to
// convert these module references because otherwise these resolve to non-existent files.
result[key] = this.convertValue(moduleName, value);
} else {
result[key] = value;
}
}
return result;
}
Expand Down
53 changes: 53 additions & 0 deletions packages/compiler-cli/test/metadata/bundler_spec.ts
Expand Up @@ -218,6 +218,59 @@ describe('metadata bundler', () => {
]);
});

it('should rewrite call expression references for static class members', () => {
const host = new MockStringBundlerHost('/', {
'lib': {
'index.ts': `export * from './deep/index';`,
'shared.ts': `
export function sharedFn() {
return {foo: true};
}`,
'deep': {
'index.ts': `
import {sharedFn} from '../shared';

export class MyClass {
static ngInjectableDef = sharedFn();
}
`,
}
}
});
const bundler = new MetadataBundler('/lib/index', undefined, host);
const bundledMetadata = bundler.getMetadataBundle().metadata;
const deepIndexMetadata = host.getMetadataFor('/lib/deep/index') !;

// The unbundled metadata should reference symbols using the relative module path.
expect(deepIndexMetadata.metadata['MyClass']).toEqual(jasmine.objectContaining({
statics: {
ngInjectableDef: {
__symbolic: 'call',
expression: {
__symbolic: 'reference',
name: 'sharedFn',
module: '../shared',
}
}
}
}));

// For the bundled metadata, the "sharedFn" symbol should not be referenced using the
// relative module path (like for unbundled), because the metadata bundle can be stored
// anywhere and it's not guaranteed that the relatively referenced files are present.
expect(bundledMetadata.metadata['MyClass']).toEqual(jasmine.objectContaining({
statics: {
ngInjectableDef: {
__symbolic: 'call',
expression: {
__symbolic: 'reference',
name: 'ɵa',
}
}
}
}));
});

it('should be able to bundle an oddly constructed library', () => {
const host = new MockStringBundlerHost('/', {
'lib': {
Expand Down