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.

….13.3,<4.5.0 in /packages/@jsii/python-runtime (#4683) Updates the requirements on [typeguard](https://github.com/agronholm/typeguard) to permit the latest version. <details> <summary>Release notes</summary> Sourced from <a href="https://github.com/agronholm/typeguard/releases">typeguard's releases</a>. <blockquote> <h2>4.4.0</h2> <ul> <li>Added proper checking for method signatures in protocol checks (<a href="https://redirect.github.com/agronholm/typeguard/pull/465">#465</a>)</li> <li>Fixed basic support for intersection protocols (<a href="https://redirect.github.com/agronholm/typeguard/pull/490">#490</a>; PR by <a href="https://github.com/antonagestam"><code>@antonagestam</code></a>)</li> <li>Fixed protocol checks running against the class of an instance and not the instance itself (this produced wrong results for non-method member checks)</li> </ul> </blockquote> </details> <details> <summary>Changelog</summary> Sourced from <a href="https://github.com/agronholm/typeguard/blob/master/docs/versionhistory.rst">typeguard's changelog</a>. <blockquote> <h1>Version history</h1> This library adheres to <code>Semantic Versioning 2.0 <https://semver.org/#semantic-versioning-200></code>_. 4.4.0 (2024-10-27) <ul> <li>Added proper checking for method signatures in protocol checks (<code>[#465](agronholm/typeguard#465) <https://github.com/agronholm/typeguard/pull/465></code>_)</li> <li>Fixed basic support for intersection protocols (<code>[#490](agronholm/typeguard#490) <https://github.com/agronholm/typeguard/pull/490></code>_; PR by <a href="https://github.com/antonagestam"><code>@antonagestam</code></a>)</li> <li>Fixed protocol checks running against the class of an instance and not the instance itself (this produced wrong results for non-method member checks)</li> </ul> 4.3.0 (2024-05-27) <ul> <li>Added support for checking against static protocols</li> <li>Fixed some compatibility problems when running on Python 3.13 (<code>[#460](agronholm/typeguard#460) <https://github.com/agronholm/typeguard/issues/460></code>_; PR by <a href="https://github.com/JelleZijlstra"><code>@JelleZijlstra</code></a>)</li> <li>Fixed test suite incompatibility with pytest 8.2 (<code>[#461](agronholm/typeguard#461) <https://github.com/agronholm/typeguard/issues/461></code>_)</li> <li>Fixed pytest plugin crashing on pytest version older than v7.0.0 (even if it's just present) (<code>[#343](agronholm/typeguard#343) <https://github.com/agronholm/typeguard/issues/343></code>_)</li> </ul> 4.2.1 (2023-03-24) <ul> <li>Fixed missing <code>typing_extensions</code> dependency for Python 3.12 (<code>[#444](agronholm/typeguard#444) <https://github.com/agronholm/typeguard/issues/444></code>_)</li> <li>Fixed deprecation warning in the test suite on Python 3.13 (<code>[#444](agronholm/typeguard#444) <https://github.com/agronholm/typeguard/issues/444></code>_)</li> </ul> 4.2.0 (2023-03-23) <ul> <li>Added support for specifying options for the pytest plugin via pytest config files (<code>[#440](agronholm/typeguard#440) <https://github.com/agronholm/typeguard/issues/440></code>_)</li> <li>Avoid creating reference cycles when type checking unions (PR by Shantanu)</li> <li>Fixed <code>Optional[...]</code> being removed from the AST if it was located within a subscript (<code>[#442](agronholm/typeguard#442) <https://github.com/agronholm/typeguard/issues/442></code>_)</li> <li>Fixed <code>TypedDict</code> from <code>typing_extensions</code> not being recognized as one (<code>[#443](agronholm/typeguard#443) <https://github.com/agronholm/typeguard/issues/443></code>_)</li> <li>Fixed <code>typing</code> types (<code>dict[str, int]</code>, <code>List[str]</code>, etc.) not passing checks against <code>type</code> or <code>Type</code> (<code>[#432](agronholm/typeguard#432) <https://github.com/agronholm/typeguard/issues/432></code>_, PR by Yongxin Wang)</li> <li>Fixed detection of optional fields (<code>NotRequired[...]</code>) in <code>TypedDict</code> when using forward references (<code>[#424](agronholm/typeguard#424) <https://github.com/agronholm/typeguard/issues/424></code>_)</li> <li>Fixed mapping checks against Django's <code>MultiValueDict</code> (<code>[#419](agronholm/typeguard#419) <https://github.com/agronholm/typeguard/issues/419></code>_)</li> </ul> 4.1.5 (2023-09-11) </blockquote> ... (truncated) </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/agronholm/typeguard/commit/efa1166c85be9a1280090fea9c287b5e4e9f3830"><code>efa1166</code></a> Added release date</li> <li><a href="https://github.com/agronholm/typeguard/commit/b72794dffe403254881ac0c327155357c43ccebf"><code>b72794d</code></a> Added proper Protocol method signature checking (<a href="https://redirect.github.com/agronholm/typeguard/issues/496">#496</a>)</li> <li><a href="https://github.com/agronholm/typeguard/commit/afad2c7b6be830900776922bb39f9346c2e77f6f"><code>afad2c7</code></a> Sorted the Ruff rules alphabetically</li> <li><a href="https://github.com/agronholm/typeguard/commit/d812f2eba9f5e898544eb4b3e597f8c38b0952e8"><code>d812f2e</code></a> Migrated to native tox TOML configuration</li> <li><a href="https://github.com/agronholm/typeguard/commit/0c50de6144d99eedb402a8e85eb8187098f8c26f"><code>0c50de6</code></a> Declared Python 3.13 support</li> <li><a href="https://github.com/agronholm/typeguard/commit/cf25d56dc0dbf6bb2f51ea29da8436b368ed4857"><code>cf25d56</code></a> Fixed annotation for typeguard_ignore() to match one for typing.no_type_check...</li> <li><a href="https://github.com/agronholm/typeguard/commit/604b08d5ba7c1b6e3d2f2ddd50dcf020f7e2794a"><code>604b08d</code></a> Use get_protocol_members in protocol checking (<a href="https://redirect.github.com/agronholm/typeguard/issues/490">#490</a>)</li> <li><a href="https://github.com/agronholm/typeguard/commit/c72b6752b7069d695898ea29abb2a31983c1bf80"><code>c72b675</code></a> [pre-commit.ci] pre-commit autoupdate (<a href="https://redirect.github.com/agronholm/typeguard/issues/471">#471</a>)</li> <li><a href="https://github.com/agronholm/typeguard/commit/ac7ac342a61db284872430c95d5e6ed7a035b7c0"><code>ac7ac34</code></a> Fixed the documentation build</li> <li><a href="https://github.com/agronholm/typeguard/commit/2c035b306996f742705da66ef64a052e715a94e2"><code>2c035b3</code></a> Assume that typing_extensions is always installed (<a href="https://redirect.github.com/agronholm/typeguard/issues/487">#487</a>)</li> <li>Additional commits viewable in <a href="https://github.com/agronholm/typeguard/compare/2.13.3...4.4.0">compare view</a></li> </ul> </details> Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details>

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

Loading

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

Loading

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

Loading

eladb Jan 3, 2019

rix0rrr Jan 3, 2019

rix0rrr Jan 3, 2019 •

edited

Loading

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 Loading

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 Loading

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 Loading

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 Loading

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

Loading

rix0rrr Jan 2, 2019 •

edited

Loading

rix0rrr Jan 3, 2019 •

edited

Loading

rix0rrr Jan 3, 2019 •

edited

Loading