feat: generate TypeScript project references #343

rix0rrr · 2019-01-02T09:49:48Z

Include TypeScript project references in the generated tsconfig.json and properly pass them to the inline compiler as well.

Creating project references enables quick (and more correct) full-repo
rebuilds, as well as allowing Visual Studio to do Find References across
all packages in the repository.

BREAKING CHANGE: all handwritten TypeScript projects that generated projects are mixed
with need composite: true configured, and a root-level index of all possible packages has to be
generated as well.

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

…TSC does natively

…ted and was giving me an error Got: TypeError: Path must be a string. Received undefined Upgrading to webpack-cli fixed it. webpack-command is deprecated as per: https://github.com/webpack-contrib/webpack-command

eladb

Goodness!

eladb · 2019-01-02T09:53:54Z

packages/jsii/lib/compiler.ts

+     * "composite: true" we consider them project references.
+     *
+     * Unfortunately it doesn't seem like the TypeScript compiler itself
+     * resolves transitive references in a way that


eladb · 2019-01-02T09:54:44Z

packages/jsii/lib/compiler.ts

+
+        // If there's no package file, don't do anything
+        if (!await fs.pathExists(packageJsonPath)) { return []; }
+        const pkg = require(packageJsonPath);


I believe we are loading package.json like 10 times already. If not too hard, maybe we can just pass that into the function?

eladb · 2019-01-02T09:55:23Z

packages/jsii/lib/compiler.ts

+        const pkg = require(packageJsonPath);
+
+        const ret = new Array<string>();
+        for (const dependencyMap of [pkg.dependencies, pkg.devDependencies]) {


peerDependencies should also be in that list I think

eladb · 2019-01-02T09:56:29Z

packages/jsii/lib/compiler.ts

+            if (dependencyMap === undefined) { continue; }
+
+            for (const depName of Object.keys(dependencyMap)) {
+                let tsconfigFile = path.join('node_modules', depName, 'tsconfig.json');


That's not necessarily going to work (for example, if npm hoists modules to an upper "node_modules").
You should use require.resolve(xxx, { paths: [ dir ] })

That is still not how that function works I believe, and all instances where we have it in our code only work because of fairy dust.

The { paths } argument to require.resolve() needs to be a list of search directories, not a list of directories from which the search algorithm will be started. In other words, it already needs to be this list:

[ '/home/local/ANT/huijbers/Temp/node_modules', '/home/local/ANT/huijbers/node_modules', '/home/local/ANT/node_modules', ... ]

I think you're supposed to use require.resolve.paths() to build this list, but for some reason that doesn't work on my machine. Investigating.

Ah yes, and require.resolve.paths(package) returns

the list of paths where we would search for package

if we were to search for package

from the JS file that contains the require.resolve.paths() command.

In fact, this seems to be what we need except it's Node 10.12+.

Going to have to construct this by hand.

I would say that we don't want project references for globally installed modules

eladb · 2019-01-02T09:59:13Z

packages/jsii/lib/compiler.ts

+     * This makes it so that running 'tsc' and running 'jsii' has the same behavior.
+     */
+    private async determineSources(files: string[]): Promise<void> {
+        this.rootFiles.splice(0);


Would it make more sense to just return the list of files instead of mutate rootFiles? Seems unnecessarily convoluted.

Brings the dependency loading phase of jsii compilation for the 'aws-sns' package on my machine down from 30s -> 2s.

- Don't load package.json again - Also crawl peerDependencies - Better NodeJS resolution mechanism - determineSources no longer compiles in-place

eladb · 2019-01-02T13:26:52Z

packages/jsii/lib/compiler.ts

+        try {
+            dep = require.resolve(depName, { paths });
+        } catch (e) {
+            // We failed to 'require' the given dependency. This might be valid if the package


You should always be able to resolve pkg/package.json (but ignoring is also fine)

cdk-build-tools for example does not point "main" to a valid JS file, so cannot be require()d.

Why not? require('cdk-build-tools/package.json') should just return the contents of it's package.json without loading the main file.

I don't think that's how require() works, but let me double-check.

Oh !@#%$ it is T_T

Well in that case I can probably go require("pkg/tsconfig.json") and save myself some hassle.

eladb · 2019-01-03T13:23:15Z

packages/jsii/lib/compiler.ts


    /**
     * Compiles the configured program.
     *
     * @param files can be specified to override the standard source code location logic. Useful for example when testing "negatives".
     */
    public async emit(...files: string[]): Promise<EmitResult | never> {
+        await this.buildTypeScriptConfig();


Any reason these are two separate functions?

Yes. They're two different operations.

Are they used elsewhere in the core individually?

No, but I don't think this is a very good argument.

Do you propose to not make helper functions at all and just inline all code of every project into a 1000-line main() function monstrosity, just because it so happens that every function is only called once?

They're separate because determining what the configuration should be, and writing the configuration to disk are two distinct operations.

eladb · 2019-01-03T13:24:33Z

packages/jsii/lib/compiler.ts


    /**
     * Compiles the configured program.
     *
     * @param files can be specified to override the standard source code location logic. Useful for example when testing "negatives".
     */
    public async emit(...files: string[]): Promise<EmitResult | never> {
+        await this.buildTypeScriptConfig();
+        await this.writeTypeScriptConfig();
+        this.rootFiles = await this.determineSources(files);


Why do we need rootFiles as a member? Can we make this a local variable?

Because I wanted to put information about the state of compilation on the Compiler object on purpose.

Rico Huijbers added 6 commits September 21, 2018 15:11

Generate project references, and make JSII compile the same files as …

bd861a9

…TSC does natively

Merge branch 'master' into huijbers/generate-project-references

ddc0cd5

Survive package.json being missing

daa8556

Update package-locks

1019ba9

Pass proper reference paths to inline compiler

eecd61d

rix0rrr requested review from RomainMuller and eladb January 2, 2019 09:49

rix0rrr requested a review from a team as a code owner January 2, 2019 09:49

rix0rrr mentioned this pull request Jan 2, 2019

chore: enable project references in build aws/aws-cdk#1466

Merged

4 tasks

eladb reviewed Jan 2, 2019

View reviewed changes

Rico Huijbers added 3 commits January 2, 2019 12:00

Add a cache for assembly loading.

c90a2d5

Brings the dependency loading phase of jsii compilation for the 'aws-sns' package on my machine down from 30s -> 2s.

Review comments

7bb080a

- Don't load package.json again - Also crawl peerDependencies - Better NodeJS resolution mechanism - determineSources no longer compiles in-place

Don't throw exception, return undefined

f78297f

eladb approved these changes Jan 3, 2019

View reviewed changes

Make project reference generation an option

2cd406b

eladb approved these changes Jan 3, 2019

View reviewed changes

Rico Huijbers added 2 commits January 3, 2019 15:09

require.resolve() a file in the package directly

7ebf06d

Correctly incorporate setting from package.json

7f0241b

rix0rrr merged commit 5eec5dc into master Jan 3, 2019

rix0rrr deleted the huijbers/generate-project-references branch January 3, 2019 14:37

RomainMuller mentioned this pull request Apr 7, 2020

feat(jsii): exclude files in tsconfig.json #1491

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: generate TypeScript project references #343

feat: generate TypeScript project references #343

rix0rrr commented Jan 2, 2019 •

edited

eladb left a comment

eladb Jan 2, 2019

eladb Jan 2, 2019

eladb Jan 2, 2019

eladb Jan 2, 2019

rix0rrr Jan 2, 2019

rix0rrr Jan 2, 2019 •

edited

rix0rrr Jan 2, 2019

eladb Jan 2, 2019

rix0rrr Jan 2, 2019

eladb Jan 2, 2019

eladb Jan 2, 2019

rix0rrr Jan 3, 2019 •

edited

eladb Jan 3, 2019

rix0rrr Jan 3, 2019

rix0rrr Jan 3, 2019 •

edited

rix0rrr Jan 3, 2019

eladb Jan 3, 2019

rix0rrr Jan 3, 2019

eladb Jan 3, 2019

rix0rrr Jan 3, 2019

eladb Jan 3, 2019

rix0rrr Jan 3, 2019

feat: generate TypeScript project references #343

feat: generate TypeScript project references #343

Conversation

rix0rrr commented Jan 2, 2019 • edited

eladb 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

rix0rrr Jan 2, 2019 • edited

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

rix0rrr Jan 3, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr Jan 3, 2019 • edited

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

rix0rrr commented Jan 2, 2019 •

edited

rix0rrr Jan 2, 2019 •

edited

rix0rrr Jan 3, 2019 •

edited

rix0rrr Jan 3, 2019 •

edited