Re-exports in ngtsc #28852

alxhub · 2019-02-20T03:56:58Z

No description provided.

AndrewKushnir

Great work @alxhub 👍 Just left a few nits. Thank you.

AndrewKushnir · 2019-02-21T00:56:09Z

packages/compiler-cli/src/ngtsc/program.ts

+    ];
+    const afterDeclarationsTransforms = [
+      declarationTransformFactory(compilation),
+      aliasTransformFactory(compilation.exportStatements),


nit: may be add a comment on why we run aliasTransformFactory again as a part of afterDeclarationsTransforms?

I actually removed it. Putting the re-exports in the .d.ts file is good behavior, but it breaks tsickle :-/.

AndrewKushnir · 2019-02-21T01:01:39Z

packages/compiler-cli/src/ngtsc/scope/src/dependency.ts

+          // source file as the re-exporting NgModule. This can happen if both a directive, its
+          // NgModule, and the re-exporting NgModule are all in the same file. In this case,
+          // no import alias is needed as it would go to the same file anyway.
+          for (const directive of exportScope.exported.directives) {


The logic to handle Directives and Pipes looks very similar. May be extract it out to a separate function?

Ahaha, I did this independently of this comment :D nice!

AndrewKushnir · 2019-02-21T01:03:41Z

packages/compiler-cli/src/ngtsc/scope/src/dependency.ts

+   * file, or in a .ts file with a handwritten definition).
+   *
+   * @param clazz the class of interest
+   * @param ngModuleImportedFrom module specifier of the import path to assume for all


Params docs don't seem to correspond actual function signature.

AndrewKushnir · 2019-02-21T01:04:45Z

packages/compiler-cli/src/ngtsc/scope/src/dependency.ts

+  private readModuleMetadataFromClass(ref: Reference<ts.Declaration>): RawDependencyMetadata|null {
+    const clazz = ref.node;
+    const resolutionContext = clazz.getSourceFile().fileName;
+    // This operation is explicitly not memoized, as it depends on `ngModuleImportedFrom`.


ngModuleImportedFrom doesn't seem to exist in the scope of this fn, old comment?

Yep, fixed.

AndrewKushnir · 2019-02-21T01:07:24Z

packages/compiler-cli/src/ngtsc/scope/src/dependency.ts

+        // Validate that the shape of the ngModuleDef type is correct.
+        ngModuleDef.type === null || !ts.isTypeReferenceNode(ngModuleDef.type) ||
+        ngModuleDef.type.typeArguments === undefined ||
+        ngModuleDef.type.typeArguments.length !== 4) {


May be add a comment on what 4 means or declare a const?

I think the comment covers it - a correctly shaped ngModuleDef type should have 4 type arguments.

AndrewKushnir · 2019-02-21T01:16:31Z

packages/compiler-cli/src/ngtsc/scope/src/local.ts

+            const expr = this.refEmitter.emit(ref.cloneWithNoIdentifiers(), sourceFile);
+            if (!(expr instanceof ExternalExpr) || expr.value.moduleName === null ||
+                expr.value.name === null) {
+              throw new Error('Expected ExternalExpr');


May be add more info into the error, like exportName, so it's easier to debug if needed?

AndrewKushnir · 2019-02-21T01:19:34Z

packages/compiler-cli/src/ngtsc/scope/src/util.ts

+    const type = element.exprName;
+    const {node, from} = reflectTypeEntityToDeclaration(type, checker);
+    if (!ts.isClassDeclaration(node)) {
+      throw new Error(`Expected ClassDeclaration`);


I'd propose to add more info into the error message, like ngModuleImportedFrom and id/name of the def if this info is available.

AndrewKushnir · 2019-02-21T01:36:14Z

packages/compiler-cli/src/ngtsc/scope/test/dependency_spec.ts

+      const exportedSymbol = symbol.exports !.get(name as ts.__String);
+      if (exportedSymbol !== undefined) {
+        const decl = exportedSymbol.valueDeclaration as ts.ClassDeclaration;
+        const specifier = /.*node_modules\/(\w+)\/index\.d\.ts$/.exec(sf.fileName) ![1];


nit: this RegExp seems identical with the one used in testHost.fileNameToModuleName fn, may be move it in a const?

AndrewKushnir · 2019-02-21T01:37:31Z

packages/compiler-cli/src/ngtsc/scope/test/dependency_spec.ts

+    const {resolver, refs} = makeTestEnv(
+        {
+          'deep': `
+        export declare class DeepDir {


nit: formatting (content should be moved to the right).

This commit splits apart selector_scope.ts in ngtsc and extracts the logic into two separate classes, the LocalModuleScopeRegistry and the DtsModuleScopeResolver. The logic is cleaned up significantly and new tests are added to verify behavior. LocalModuleScopeRegistry implements the NgModule semantics for compilation scopes, and handles NgModules declared in the current compilation unit. DtsModuleScopeResolver implements simpler logic for export scopes and handles NgModules declared in .d.ts files. This is done in preparation for the addition of re-export logic to solve StrictDeps issues.

In certain configurations (such as the g3 repository) which have lots of small compilation units as well as strict dependency checking on generated code, ngtsc's default strategy of directly importing directives/pipes into components will not work. To handle these cases, an additional mode is introduced, and is enabled when using the FileToModuleHost provided by such compilation environments. In this mode, when ngtsc encounters an NgModule which re-exports another from a different file, it will re-export all the directives it contains at the ES2015 level. The exports will have a predictable name based on the FileToModuleHost. For example, if the host says that a directive Foo is from the 'root/external/foo' module, ngtsc will add: ``` export {Foo as ɵng$root$external$foo$$Foo} from 'root/external/foo'; ``` Consumers of the re-exported directive will then import it via this path instead of directly from root/external/foo, preserving strict dependency semantics.

alxhub · 2019-02-22T20:08:50Z

Caretaker: I am the approver for compiler/ngcc. This PR is otherwise approved by @AndrewKushnir :)

…rts (#28852) In certain configurations (such as the g3 repository) which have lots of small compilation units as well as strict dependency checking on generated code, ngtsc's default strategy of directly importing directives/pipes into components will not work. To handle these cases, an additional mode is introduced, and is enabled when using the FileToModuleHost provided by such compilation environments. In this mode, when ngtsc encounters an NgModule which re-exports another from a different file, it will re-export all the directives it contains at the ES2015 level. The exports will have a predictable name based on the FileToModuleHost. For example, if the host says that a directive Foo is from the 'root/external/foo' module, ngtsc will add: ``` export {Foo as ɵng$root$external$foo$$Foo} from 'root/external/foo'; ``` Consumers of the re-exported directive will then import it via this path instead of directly from root/external/foo, preserving strict dependency semantics. PR Close #28852

angular-automatic-lock-bot · 2019-09-14T17:53:08Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Feb 20, 2019

alxhub force-pushed the ngtsc-reexports branch from 499fc1d to 10c8496 Compare February 20, 2019 17:57

benlesh added area: core Issues related to the framework runtime comp: ivy labels Feb 20, 2019

ngbot bot added this to the needsTriage milestone Feb 20, 2019

AndrewKushnir approved these changes Feb 21, 2019

View reviewed changes

alxhub force-pushed the ngtsc-reexports branch 2 times, most recently from ea6f022 to d8d8bb3 Compare February 21, 2019 22:30

alxhub added 2 commits February 22, 2019 10:10

alxhub force-pushed the ngtsc-reexports branch from d8d8bb3 to d4e743f Compare February 22, 2019 18:12

alxhub added the target: major This PR is targeted for the next major release label Feb 22, 2019

alxhub marked this pull request as ready for review February 22, 2019 19:46

alxhub requested review from a team as code owners February 22, 2019 19:46

alxhub added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Feb 22, 2019

benlesh closed this in 15c065f Feb 22, 2019

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-exports in ngtsc #28852

Re-exports in ngtsc #28852

alxhub commented Feb 20, 2019

AndrewKushnir left a comment

AndrewKushnir Feb 21, 2019

alxhub Feb 22, 2019

AndrewKushnir Feb 21, 2019

alxhub Feb 22, 2019

AndrewKushnir Feb 21, 2019

alxhub Feb 22, 2019

AndrewKushnir Feb 21, 2019

alxhub Feb 22, 2019

AndrewKushnir Feb 21, 2019

alxhub Feb 22, 2019

AndrewKushnir Feb 21, 2019

alxhub Feb 22, 2019

AndrewKushnir Feb 21, 2019

alxhub Feb 22, 2019

AndrewKushnir Feb 21, 2019

alxhub Feb 22, 2019

AndrewKushnir Feb 21, 2019

alxhub Feb 22, 2019

alxhub commented Feb 22, 2019

angular-automatic-lock-bot bot commented Sep 14, 2019

Re-exports in ngtsc #28852

Re-exports in ngtsc #28852

Conversation

alxhub commented Feb 20, 2019

AndrewKushnir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alxhub commented Feb 22, 2019

angular-automatic-lock-bot bot commented Sep 14, 2019