From e382632473dc92b6292ec264c9120bd07ca926bd Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Thu, 16 Jul 2020 14:33:11 +0200 Subject: [PATCH] build: fix symbol extractor not dealing with ES2015 classes (#38093) We recently reworked our `ng_rollup_bundle` rule to no longer output ESM5 and to optimize applications properly (previously applications were not optimized properly due to incorrect build optimizer setup). This change meant that a lot of symbols have been removed from the golden correctly. See: fd65958b887f6ea8dd5235e6de1d533e4c578602 Unfortunately though, a few symbols have been accidentally removed because they are now part of the bundle as ES2015 classes which the symbol extractor does not pick up. This commit fixes the symbol extractor to capture ES2015 classes. We also update the golden to reflect this change. PR Close #38093 --- .../cyclic_import/bundle.golden_symbols.json | 6 +++ .../hello_world/bundle.golden_symbols.json | 6 +++ .../injection/bundle.golden_symbols.json | 9 ++++ .../bundling/todo/bundle.golden_symbols.json | 45 +++++++++++++++++++ tools/symbol-extractor/BUILD.bazel | 2 + tools/symbol-extractor/symbol_extractor.ts | 4 ++ .../symbol-extractor/symbol_extractor_spec.ts | 42 ++++++++++++----- .../symbol_extractor_spec/BUILD.bazel | 22 +++++++++ .../es2015_class_output.json | 4 ++ .../es2015_class_output.ts | 19 ++++++++ 10 files changed, 148 insertions(+), 11 deletions(-) create mode 100644 tools/symbol-extractor/symbol_extractor_spec/BUILD.bazel create mode 100644 tools/symbol-extractor/symbol_extractor_spec/es2015_class_output.json create mode 100644 tools/symbol-extractor/symbol_extractor_spec/es2015_class_output.ts diff --git a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json index d012b3be33..c2ba0e34b1 100644 --- a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json +++ b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json @@ -35,6 +35,12 @@ { "name": "NO_CHANGE" }, + { + "name": "NodeInjectorFactory" + }, + { + "name": "SimpleChange" + }, { "name": "TriggerComponent" }, diff --git a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json index f7d1fe5780..0777fe91e6 100644 --- a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json @@ -29,6 +29,12 @@ { "name": "NO_CHANGE" }, + { + "name": "NodeInjectorFactory" + }, + { + "name": "SimpleChange" + }, { "name": "ViewEncapsulation" }, diff --git a/packages/core/test/bundling/injection/bundle.golden_symbols.json b/packages/core/test/bundling/injection/bundle.golden_symbols.json index c404a5d0db..4dd6420bd2 100644 --- a/packages/core/test/bundling/injection/bundle.golden_symbols.json +++ b/packages/core/test/bundling/injection/bundle.golden_symbols.json @@ -17,6 +17,9 @@ { "name": "InjectFlags" }, + { + "name": "InjectionToken" + }, { "name": "NEW_LINE" }, @@ -44,9 +47,15 @@ { "name": "NULL_INJECTOR" }, + { + "name": "NullInjector" + }, { "name": "Optional" }, + { + "name": "R3Injector" + }, { "name": "ScopedService" }, diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json index 0d6e14706b..621068548e 100644 --- a/packages/core/test/bundling/todo/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo/bundle.golden_symbols.json @@ -5,6 +5,12 @@ { "name": "ChangeDetectionStrategy" }, + { + "name": "DefaultIterableDiffer" + }, + { + "name": "DefaultIterableDifferFactory" + }, { "name": "EMPTY_ARRAY" }, @@ -17,9 +23,15 @@ { "name": "ElementRef" }, + { + "name": "ErrorHandler" + }, { "name": "InjectFlags" }, + { + "name": "IterableChangeRecord_" + }, { "name": "IterableDiffers" }, @@ -53,12 +65,30 @@ { "name": "NgForOf" }, + { + "name": "NgForOfContext" + }, { "name": "NgIf" }, + { + "name": "NgIfContext" + }, + { + "name": "NgModuleRef" + }, + { + "name": "NodeInjector" + }, + { + "name": "NodeInjectorFactory" + }, { "name": "Optional" }, + { + "name": "RecordViewTuple" + }, { "name": "SWITCH_ELEMENT_REF_FACTORY" }, @@ -68,6 +98,9 @@ { "name": "SWITCH_VIEW_CONTAINER_REF_FACTORY" }, + { + "name": "SimpleChange" + }, { "name": "SkipSelf" }, @@ -92,6 +125,9 @@ { "name": "ToDoAppComponent_section_5_li_3_input_6_Template" }, + { + "name": "Todo" + }, { "name": "TodoStore" }, @@ -101,9 +137,18 @@ { "name": "ViewEncapsulation" }, + { + "name": "ViewRef" + }, { "name": "_CLEAN_PROMISE" }, + { + "name": "_DuplicateItemRecordList" + }, + { + "name": "_DuplicateMap" + }, { "name": "__forward_ref__" }, diff --git a/tools/symbol-extractor/BUILD.bazel b/tools/symbol-extractor/BUILD.bazel index 8cd5f39c3e..057c7cc6c0 100644 --- a/tools/symbol-extractor/BUILD.bazel +++ b/tools/symbol-extractor/BUILD.bazel @@ -37,5 +37,7 @@ jasmine_node_test( data = glob(["symbol_extractor_spec/**"]), deps = [ ":test_lib", + "//tools/symbol-extractor/symbol_extractor_spec:es2015_class_output", + "//tools/symbol-extractor/symbol_extractor_spec:fixtures", ], ) diff --git a/tools/symbol-extractor/symbol_extractor.ts b/tools/symbol-extractor/symbol_extractor.ts index 60770560cf..0c781bbde8 100644 --- a/tools/symbol-extractor/symbol_extractor.ts +++ b/tools/symbol-extractor/symbol_extractor.ts @@ -58,6 +58,10 @@ export class SymbolExtractor { const funcDecl = child as ts.FunctionDeclaration; funcDecl.name && symbols.push({name: stripSuffix(funcDecl.name.getText())}); break; + case ts.SyntaxKind.ClassDeclaration: + const classDecl = child as ts.ClassDeclaration; + classDecl.name && symbols.push({name: stripSuffix(classDecl.name.getText())}); + break; default: // Left for easier debugging. // console.log('###', ts.SyntaxKind[child.kind], child.getText()); diff --git a/tools/symbol-extractor/symbol_extractor_spec.ts b/tools/symbol-extractor/symbol_extractor_spec.ts index 2942c94af9..b8fe910149 100644 --- a/tools/symbol-extractor/symbol_extractor_spec.ts +++ b/tools/symbol-extractor/symbol_extractor_spec.ts @@ -8,31 +8,51 @@ import * as fs from 'fs'; import * as path from 'path'; -import * as ts from 'typescript'; -import {Symbol, SymbolExtractor} from './symbol_extractor'; +import {SymbolExtractor} from './symbol_extractor'; describe('scenarios', () => { const symbolExtractorSpecDir = path.dirname( require.resolve('angular/tools/symbol-extractor/symbol_extractor_spec/empty.json')); const scenarioFiles = fs.readdirSync(symbolExtractorSpecDir); - for (let i = 0; i < scenarioFiles.length; i = i + 2) { - let jsFile = scenarioFiles[i]; - let jsonFile = scenarioFiles[i + 1]; - let testName = jsFile.substring(0, jsFile.lastIndexOf('.')); - if (!jsFile.endsWith('.js')) throw new Error('Expected: .js file found: ' + jsFile); - if (!jsonFile.endsWith('.json')) throw new Error('Expected: .json file found: ' + jsonFile); + for (let i = 0; i < scenarioFiles.length; i++) { + const filePath = scenarioFiles[i]; + // We only consider files as tests if they have a `.js` extension, but do + // not resolve to a tsickle externs file (which is a leftover from TS targets). + if (!filePath.endsWith('.js') || filePath.endsWith('.externs.js')) { + continue; + } + const testName = filePath.substring(0, filePath.lastIndexOf('.')); + const goldenFilePath = path.join(symbolExtractorSpecDir, `${testName}.json`); + + if (!fs.existsSync(goldenFilePath)) { + throw new Error(`No golden file found for test: ${filePath}`); + } // Left here so that it is easy to debug single test. // if (testName !== 'hello_world_min_debug') continue; it(testName, () => { - const jsFileContent = fs.readFileSync(path.join(symbolExtractorSpecDir, jsFile)).toString(); - const jsonFileContent = - fs.readFileSync(path.join(symbolExtractorSpecDir, jsonFile)).toString(); + const jsFileContent = fs.readFileSync(path.join(symbolExtractorSpecDir, filePath)).toString(); + const jsonFileContent = fs.readFileSync(goldenFilePath).toString(); const symbols = SymbolExtractor.parse(testName, jsFileContent); const diff = SymbolExtractor.diff(symbols, jsonFileContent); expect(diff).toEqual({}); }); } + + // Tests not existing in source root. We cannot glob for generated test fixtures as + // tests do not run in a sandbox on Windows. + + it('should properly capture classes in TypeScript ES2015 class output', () => { + const jsFileContent = fs.readFileSync( + require.resolve( + 'angular/tools/symbol-extractor/symbol_extractor_spec/es2015_class_output.mjs'), + 'utf8'); + const jsonFileContent = + fs.readFileSync(path.join(symbolExtractorSpecDir, 'es2015_class_output.json')).toString(); + const symbols = SymbolExtractor.parse('es2015_class_output', jsFileContent); + const diff = SymbolExtractor.diff(symbols, jsonFileContent); + expect(diff).toEqual({}); + }); }); diff --git a/tools/symbol-extractor/symbol_extractor_spec/BUILD.bazel b/tools/symbol-extractor/symbol_extractor_spec/BUILD.bazel new file mode 100644 index 0000000000..220b271b77 --- /dev/null +++ b/tools/symbol-extractor/symbol_extractor_spec/BUILD.bazel @@ -0,0 +1,22 @@ +load("//tools:defaults.bzl", "ts_library") + +package(default_visibility = ["//visibility:public"]) + +ts_library( + name = "es2015_class_output_lib", + srcs = ["es2015_class_output.ts"], +) + +filegroup( + name = "es2015_class_output", + srcs = [":es2015_class_output_lib"], + output_group = "es6_sources", +) + +filegroup( + name = "fixtures", + srcs = glob([ + "**/*.js", + "**/*.json", + ]), +) diff --git a/tools/symbol-extractor/symbol_extractor_spec/es2015_class_output.json b/tools/symbol-extractor/symbol_extractor_spec/es2015_class_output.json new file mode 100644 index 0000000000..c4e8c4de75 --- /dev/null +++ b/tools/symbol-extractor/symbol_extractor_spec/es2015_class_output.json @@ -0,0 +1,4 @@ +[ + "HelloWorld", + "WithStaticMembers" +] diff --git a/tools/symbol-extractor/symbol_extractor_spec/es2015_class_output.ts b/tools/symbol-extractor/symbol_extractor_spec/es2015_class_output.ts new file mode 100644 index 0000000000..787624f259 --- /dev/null +++ b/tools/symbol-extractor/symbol_extractor_spec/es2015_class_output.ts @@ -0,0 +1,19 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +class HelloWorld { + greet() { + console.info('Hello!'); + } +} + +// TypeScript generates different output for classes with +// static members. +class WithStaticMembers extends HelloWorld { + static message = 'literal value'; +}