Skip to content

Commit

Permalink
fix: dependency submodules may not be discovered (#3151)
Browse files Browse the repository at this point in the history
When a dependency exposes submodules, and the entry point of that
dependency is never imported by the program being compiled, symbols
from the dependency were not correctly mapped to their submodule because
the entry point was not part of the compiler input (and hence the
`ts.Program` did not hold a `ts.SourceFile` for it, and the
`ts.TypeChecker` did not have symbols for exports therein).

In order to address this problem, this always explicitly references
dependencies' entry point (the `.d.ts` file referenced by the `types`
key) to the compiler input path. This guarantees the compiler binds
symbols within those, and that the submodule mapper can appropriately
discover those.

------------------------------------------------------------------------

An alternative approach was considered (but rejected because it
introduced significant complexity): we could have created
`ts.SourceFile` objects using `ts.createSourceFile`, which allows us to
obtain a set of `ts.Statement` nodes, however those are not bound to
symbols and we then have to process the AST without the assistance of
the type checker (and would end up having to re-implement a sub-set of
the binder/type-checker's features).



---

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

[Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0
  • Loading branch information
Romain Marcadier committed Nov 10, 2021
1 parent ba4a20d commit 5768bb9
Show file tree
Hide file tree
Showing 14 changed files with 140 additions and 8 deletions.
3 changes: 2 additions & 1 deletion lerna.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
"npmClient": "yarn",
"useWorkspaces": true,
"packages": [
"packages/*"
"packages/*",
"regression-tests/@*/*"
],
"command": {
"bootstrap": {
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
"packages/*",
"packages/@jsii/*",
"packages/@scope/*",
"regression-tests/@*/*",
"tools/*"
],
"nohoist": [
Expand Down
8 changes: 7 additions & 1 deletion packages/jsii/lib/assembler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3213,7 +3213,13 @@ function inferRootDir(program: ts.Program): string | undefined {
.map((fileName) =>
path.relative(program.getCurrentDirectory(), path.dirname(fileName)),
)
.map(segmentPath);
.map(segmentPath)
// Dependency entry points are in this path, and they MAY resolve from the
// same mono-repo, in which case they won't appear to be external libraries,
// as there may not be a `/node_modules/` segment in their canonical path.
// They well however come from a parent directory, so their path segments
// will start with "..".
.filter(([head]) => head !== '..');

const maxPrefix = Math.min(
...directories.map((segments) => segments.length - 1),
Expand Down
41 changes: 35 additions & 6 deletions packages/jsii/lib/compiler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,11 +133,15 @@ export class Compiler implements Emitter {
const pi = this.options.projectInfo;
const projectRoot = pi.projectRoot;
const host = ts.createWatchCompilerHost(
this.configPath,
this.rootFiles,
{
...pi.tsc,
...BASE_COMPILER_OPTIONS,
noEmitOnError: false,
tsBuildInfoFile: path.join(
pi.tsc?.outDir ?? pi.tsc?.rootDir ?? pi.projectRoot,
'tsconfig.tsbuildinfo',
),
},
{
...ts.sys,
Expand All @@ -148,6 +152,7 @@ export class Compiler implements Emitter {
ts.createEmitAndSemanticDiagnosticsBuilderProgram,
opts?.reportDiagnostics,
opts?.reportWatchStatus,
this.typescriptConfig?.references,
);
if (!host.getDefaultLibLocation) {
throw new Error(
Expand Down Expand Up @@ -495,23 +500,47 @@ export class Compiler implements Emitter {
* This makes it so that running 'tsc' and running 'jsii' has the same behavior.
*/
private determineSources(files: string[]): string[] {
const ret = new Array<string>();
const ret = new Set<string>();

if (files.length > 0) {
ret.push(...files);
for (const file of files) {
ret.add(path.resolve(this.options.projectInfo.projectRoot, file));
}
} else {
const parseConfigHost = parseConfigHostFromCompilerHost(
this.compilerHost,
);
const parsed = ts.parseJsonConfigFileContent(
// Note: the fileNames here are resolved by the parseConfigHost.
const { fileNames } = ts.parseJsonConfigFileContent(
this.typescriptConfig,
parseConfigHost,
this.options.projectInfo.projectRoot,
);
ret.push(...parsed.fileNames);
for (const file of fileNames) {
ret.add(file);
}
}

return ret;
// Bonus: ensure all dependencies' entry points are included in the compiler
// input path. This guarantees we have symbols for all types, from the
// module root, which is necessary in order to properly detect submodules.
for (const assm of this.options.projectInfo.dependencyClosure) {
const { resolvedModule } = ts.resolveModuleName(
assm.name,
path.join(
this.options.projectInfo.projectRoot,
this.options.projectInfo.types,
),
this.typescriptConfig?.compilerOptions ?? {},
ts.sys,
);
if (!resolvedModule) {
continue;
}
ret.add(resolvedModule.resolvedFileName);
}

return Array.from(ret);
}

/**
Expand Down
4 changes: 4 additions & 0 deletions regression-tests/@barrelimports/consumer/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
/.jsii
/tsconfig.json
*.js
*.d.ts
6 changes: 6 additions & 0 deletions regression-tests/@barrelimports/consumer/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# @barrelimport/consumer

This library re-exports a type imported from a barrel location within
`@barrelimport/provider`, without actually importing `@barrelimport/provider`
itself, so we validate the jsii compiler correctly identifies the submodule
declarations.
6 changes: 6 additions & 0 deletions regression-tests/@barrelimports/consumer/lib/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// Directly importing NamespacedStruct, NEVER having imported @barrelimports/provider
import { NamespacedStruct } from '@barrelimports/provider/lib/namespaced';

export class UsingBarrelImport {
public constructor(public readonly props: NamespacedStruct) { }
}
30 changes: 30 additions & 0 deletions regression-tests/@barrelimports/consumer/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
{
"name": "@barrelimports/consumer",
"version": "0.0.0",
"private": true,
"license": "Apache-2.0",
"main": "lib/index.js",
"types": "lib/index.d.ts",
"author": {
"name": "Amazon Web Services",
"url": "https://aws.amazon.com"
},
"repository": {
"type": "git",
"url": "https://github.com/aws/jsii.git",
"directory": "regression-tests/@barrelimports/provider"
},
"scripts": {
"build": "jsii"
},
"peerDependencies": {
"@barrelimports/provider": "0.0.0"
},
"devDependencies": {
"@barrelimports/provider": "0.0.0",
"jsii": "^0.0.0"
},
"jsii": {
"targets": {}
}
}
4 changes: 4 additions & 0 deletions regression-tests/@barrelimports/provider/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
/.jsii
/tsconfig.json
*.js
*.d.ts
5 changes: 5 additions & 0 deletions regression-tests/@barrelimports/provider/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# @barrelimport/provider

This library provides a single namespaced export struct, so that we can validate
the jsii compiler is correctly able to identify submodule declarations when the
package entry point is never imported by the consuming code.
1 change: 1 addition & 0 deletions regression-tests/@barrelimports/provider/lib/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * as namespaced from './namespaced';
3 changes: 3 additions & 0 deletions regression-tests/@barrelimports/provider/lib/namespaced.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export interface NamespacedStruct {
readonly name: string;
}
26 changes: 26 additions & 0 deletions regression-tests/@barrelimports/provider/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
{
"name": "@barrelimports/provider",
"version": "0.0.0",
"private": true,
"license": "Apache-2.0",
"main": "lib/index.js",
"types": "lib/index.d.ts",
"author": {
"name": "Amazon Web Services",
"url": "https://aws.amazon.com"
},
"repository": {
"type": "git",
"url": "https://github.com/aws/jsii.git",
"directory": "regression-tests/@barrelimports/provider"
},
"scripts": {
"build": "jsii"
},
"devDependencies": {
"jsii": "^0.0.0"
},
"jsii": {
"targets": {}
}
}
10 changes: 10 additions & 0 deletions regression-tests/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# Regression Tests

This directory contains packages that are supposed to cleanly compile using
`jsii`. This validates the compiler is able to correctly interpret the source.

When adding new tests, be sure to follow these guidelines:

- Create a new namespace for each regression scenario
- All packages should have `private: true` in their `package.json`
- Provide a README.md with an explanation of what the package(s) validate

0 comments on commit 5768bb9

Please sign in to comment.