fix(jsii): dependency submodules are not tagged #1663

RomainMuller · 2020-05-13T11:31:54Z

Commit Message

fix(jsii): dependency submodules are not tagged (#1663)

The Assembler used to ignore any export that was coined as an external
library import by the TypeScript resolver. However, when traversing
dependencies to collect their submodule structure, everything is an
external library import (which basically is a check that the import
path has a /node_modules/ in it).

This commit changes the way external imports are skipped so that when
investigating submodules of a particular dependency, a different logic
is used to determine whether they are external (checks that the file
is under the same root; and that it is not under a /node_modules/
subdirectory of that root).

Note:
This is difficult to test for in the contect of a mono-repo, as it
requires an external dependency (dependencies within the mono-repo are
resolved to their "canonical" path, which does not include the
/node_modules/ path element, and thus is not reported as an external
library import, apparently).

End Commit Message

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

The Assembler used to ignore any export that was coined as an external library import by the TypeScript resolver. However, when traversing dependencies to collect their submodule structure, everything is an external library import (which basically is a check that the import path has a `/node_modules/` in it). This commit changes the way external imports are skipped so that when investigating submodules of a particular dependency, a different logic is used to determine whether they are external (checks that the file is under the same root; and that it is not under a `/node_modules/` subdirectory of that root). > **Note:** > This is difficult to test for in the contect of a mono-repo, as it > requires an external dependency (dependencies within the mono-repo are > resolved to their "canonical" path, which does not include the > `/node_modules/` path element, and thus is not reported as an external > library import, apparently).

aws-cdk-automation · 2020-05-13T11:45:54Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-Blkkw9bQFn8A
Commit ID: 06fe294
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-05-13T11:47:29Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-Blkkw9bQFn8A
Commit ID: 59d8b05
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

MrArnoldPalmer

🔧

NetaNir

I have added some questions, nothing blocking mostly for my understanding as I have been spending time in this codepath lately :)

NetaNir · 2020-05-14T04:29:00Z

packages/jsii/lib/assembler.ts

-    if (resolution.resolvedModule.isExternalLibraryImport) {
+    if (
+      // We're not looking into a dependency's namespace exports, and the resolution says it's external
+      (packageRoot === this.projectInfo.projectRoot && resolution.resolvedModule.isExternalLibraryImport)


Can this two conditions ever happen together?

Yes. They'll do in the phase where we're actually compiling our project (as opposed to crawling dependencies).

NetaNir · 2020-05-14T04:44:50Z

packages/jsii/lib/assembler.ts

+      // Or the module is under one the current dependency's node_modules subtree
+      || resolution.resolvedModule.resolvedFileName.split(path.sep).filter(entry => entry === 'node_modules').length
+          !== packageRoot.split(path.sep).filter(entry => entry === 'node_modules').length
+    ) {


Help me out here, why does a different number of node_modules folder in a path mean we don't want to register it?

When we reach here, we've already established that the file resolved within the same root (foo/node_modules/<dependency>/<some_file>). This means the file belongs do <dependency>, unless it happens to be a dependency that wasn't de-duplicated up (foo/node_modules/<dependency>/node_modules/<dependency-of-dependency>), at which point this is effectively a re-export of another module, and we don't want to duplicate those types (it'd be SUPER awkward in Java & C# at the very least)

NetaNir · 2020-05-14T04:55:35Z

packages/jsii/lib/assembler.ts

+      // We're not looking into a dependency's namespace exports, and the resolution says it's external
+      (packageRoot === this.projectInfo.projectRoot && resolution.resolvedModule.isExternalLibraryImport)
+      // Or the module resolves outside of the current dependency's tree entirely
+      || !resolution.resolvedModule.resolvedFileName.startsWith(packageRoot)
+      // Or the module is under one the current dependency's node_modules subtree
+      || resolution.resolvedModule.resolvedFileName.split(path.sep).filter(entry => entry === 'node_modules').length
+          !== packageRoot.split(path.sep).filter(entry => entry === 'node_modules').length
+    ) {
      // External re-exports are "pure-javascript" sugar; they need not be
      // represented in the jsii Assembly since the types in there will be
      // resolved through dependencies.
      return;
    }


To give me more context, is this if an optimization? If not, what are we preventing?

We are preventing creating a second name for the same actual type, because while TypeScript will totally follow the re-export and understand the type is just being aliased; other languages that do not feature structural type-checking will consider the two instances of the type as entirely distinct entities - that is no bueno.

mergify · 2020-05-14T05:12:22Z

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

mergify · 2020-05-14T05:12:59Z

Merging (with squash)...

aws-cdk-automation · 2020-05-14T05:27:39Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-Blkkw9bQFn8A
Commit ID: fd4f17b
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-05-14T08:01:04Z

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

mergify · 2020-05-14T08:01:41Z

Merging (with squash)...

RomainMuller requested a review from NetaNir May 13, 2020 11:32

RomainMuller self-assigned this May 13, 2020

mergify bot added the contribution/core This is a PR that came from AWS. label May 13, 2020

RomainMuller added bug This issue is a bug. module/compiler Issues affecting the JSII compiler and removed contribution/core This is a PR that came from AWS. labels May 13, 2020

Merge branch 'master' into rmuller/fix-external-import-detection

59d8b05

RomainMuller added the effort/small Small work item – less than a day of effort label May 13, 2020

MrArnoldPalmer approved these changes May 13, 2020

View reviewed changes

NetaNir reviewed May 14, 2020

View reviewed changes

mergify bot added the pr/ready-to-merge This PR is ready to be merged. label May 14, 2020

NetaNir removed the pr/ready-to-merge This PR is ready to be merged. label May 14, 2020

Merge branch 'master' into rmuller/fix-external-import-detection

fd4f17b

NetaNir added the pr/do-not-merge This PR should not be merged at this time. label May 14, 2020

RomainMuller removed the pr/do-not-merge This PR should not be merged at this time. label May 14, 2020

mergify bot added the pr/ready-to-merge This PR is ready to be merged. label May 14, 2020

mergify bot merged commit 18e3702 into master May 14, 2020

mergify bot deleted the rmuller/fix-external-import-detection branch May 14, 2020 08:01

mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label May 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(jsii): dependency submodules are not tagged #1663

fix(jsii): dependency submodules are not tagged #1663

RomainMuller commented May 13, 2020 •

edited

Loading

aws-cdk-automation commented May 13, 2020

aws-cdk-automation commented May 13, 2020

MrArnoldPalmer left a comment

NetaNir left a comment

NetaNir May 14, 2020

RomainMuller May 14, 2020

NetaNir May 14, 2020

RomainMuller May 14, 2020

NetaNir May 14, 2020

RomainMuller May 14, 2020

mergify bot commented May 14, 2020

mergify bot commented May 14, 2020

aws-cdk-automation commented May 14, 2020

mergify bot commented May 14, 2020

mergify bot commented May 14, 2020

fix(jsii): dependency submodules are not tagged #1663

fix(jsii): dependency submodules are not tagged #1663

Conversation

RomainMuller commented May 13, 2020 • edited Loading

Commit Message

End Commit Message

aws-cdk-automation commented May 13, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented May 13, 2020

AWS CodeBuild CI Report

MrArnoldPalmer left a comment

Choose a reason for hiding this comment

NetaNir left a comment

Choose a reason for hiding this comment

NetaNir May 14, 2020

Choose a reason for hiding this comment

RomainMuller May 14, 2020

Choose a reason for hiding this comment

NetaNir May 14, 2020

Choose a reason for hiding this comment

RomainMuller May 14, 2020

Choose a reason for hiding this comment

NetaNir May 14, 2020

Choose a reason for hiding this comment

RomainMuller May 14, 2020

Choose a reason for hiding this comment

mergify bot commented May 14, 2020

mergify bot commented May 14, 2020

aws-cdk-automation commented May 14, 2020

AWS CodeBuild CI Report

mergify bot commented May 14, 2020

mergify bot commented May 14, 2020

RomainMuller commented May 13, 2020 •

edited

Loading