feat: enable TypeScript project references #5155

raymondfeng · 2020-04-19T20:35:51Z

See https://www.typescriptlang.org/docs/handbook/project-references.html

Supersedes #1636
Implements #2609

TypeScript project references seem to be working with latest VSCode now. Please help verify.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

bajtos

Thank you @raymondfeng for starting this initiative, I am looking forward to have project references enabled 👏

Did you run the steps listed in How to test infrastructure changes? Are there any issues to be aware of?

Please make sure to update that section of our dev docs to accurately describe the new setup.

What is the impact of this change on the build time?

Full build from clean? npm run clean && time npm run build
Incremental build after no (or a small) change was made? time npm run build

With project references in place, we should get rid of top-level manually-written index.js, index.d.ts and index.ts files. Their purpose is to trick IDE into treating the entire monorepo as a single TypeScript project. That's no longer desirable, we want each package to be treated as its own project. Those index files are also causing slow build times. As I shown in #2613, we can speed up our build by getting rid of them.

.travis.yml

bin/update-ts-project-refs.js

packages/build/bin/compile-package.js

packages/build/config/tsconfig.common.json

raymondfeng · 2020-04-20T16:09:46Z

Some smoke test results for performance:

With this PR

First-time (clean build)

real	0m30.565s
user	0m37.962s
sys	0m3.128s

Second-time (incremental build)

real	0m1.476s
user	0m0.960s
sys	0m0.420s

Without this PR

First-time

real	2m1.125s
user	7m11.410s
sys	0m24.406s

Second-time

real	1m12.558s
user	3m45.993s
sys	0m20.833s

The improvement is significant.

raymondfeng · 2020-04-20T16:48:22Z

With project references in place, we should get rid of top-level manually-written index.js, index.d.ts and index.ts files. Their purpose is to trick IDE into treating the entire monorepo as a single TypeScript project. That's no longer desirable, we want each package to be treated as its own project. Those index files are also causing slow build times. As I shown in #2613, we can speed up our build by getting rid of them.

I would like to defer this change to a follow-up PR. One of the caveats listed in the project references is that we have to run tsc -b first. I'm not sure if it's still true. See https://www.typescriptlang.org/docs/handbook/project-references.html#caveats-for-project-references.

raymondfeng · 2020-04-20T16:49:28Z

Did you run the steps listed in How to test infrastructure changes? Are there any issues to be aware of?

Yes. I did.

bajtos · 2020-04-21T05:07:49Z

Some smoke test results for performance:
(...)
The improvement is significant.

This is awesome, even better than I expected.

I would like to defer this change to a follow-up PR. One of the caveats listed in the project references is that we have to run tsc -b first. I'm not sure if it's still true. See https://www.typescriptlang.org/docs/handbook/project-references.html#caveats-for-project-references.

Yeah, I was thinking about deferring this part too. The current PR is large enough, it's better to work incrementally. I am happy to take a look at index changes myself after this PR is landed.

Did you run the steps listed in How to test infrastructure changes? Are there any issues to be aware of?

Yes. I did.

Cool 👏 Did you find any issues? Does everything work as before?

bajtos · 2020-04-21T05:14:54Z

packages/build/bin/compile-package.js

+
+/**
+ * `tsc -b` only accepts valid arguments. `npm run build -- --<other-arg>` may
+ * pass in extra arguments. We need to remove such arguments.


What kind of arguments are getting in via npm run build -- --<other-arg>? If they are provided by the user (either on command line or via their package.json) then we must let them know about the problem and especially which parameters are removed by lb-tsc.

Personally, I don't think it's a good idea to add such parameter-modifications to lb-tsc, I am concerned about support and maintenance costs. IMO, we should let tsc fail the build and expect the user to fix the parameters passed tolb-tsc/tsc.

bajtos · 2020-04-21T05:25:18Z

BTW, lb-tsc provides --copy-resources option. I don't think that's going to work with project references. IIUC, lb-tsc is invoked for the top-level monorepo-root "project" only, that calls tsc to run the build and after that individual packages are built using tsc. It's not a problem in this monorepo, because we have removed usage of that flag long time ago, but it's something to be aware of. I am not sure if there is anything we can do about that, because when we run lb-tsc for the top-level project, then it's difficult to know what lb-tsc arguments are configured by individual packages. Maybe we can put a warning to our current lb-tsc docs? (I am not sure where the docs is.)

raymondfeng · 2020-04-21T05:35:57Z

The removal of invalid options is for npm script composition as other npm runs can trigger npm run build but npm always pass extra args to all commands.

bajtos · 2020-04-21T05:40:20Z

The removal of invalid options is for npm script composition as other npm runs can trigger npm run build but npm always pass extra args to all commands.

I see, do you have any example?

I am concerned that some day, a developer will add a new option to npm run build script, lb-tsc will silently discard it, and the poor developer will have hard times figuring out what's going on and why their new switch is ignored. Can you please find a way how to let developers know about flags removed by the tool? E.g. use console.warn or debug logs.

raymondfeng · 2020-04-21T06:12:39Z

I see, do you have any example?

I am concerned that some day, a developer will add a new option to npm run build script, lb-tsc will silently discard it, and the poor developer will have hard times figuring out what's going on and why their new switch is ignored. Can you please find a way how to let developers know about flags removed by the tool? E.g. use console.warn or debug logs.

It can reproduced by cd packages/context; npm test -- --timeout 5000.

There are debug statements to show how loopback:build is working.

raymondfeng · 2020-04-21T06:15:43Z

I'm not sure why Travis build fails for some of the acceptance tests - https://github.com/strongloop/loopback-next/pull/5155/checks?check_run_id=603795936.

bajtos · 2020-04-21T07:59:32Z

It can reproduced by cd packages/context; npm test -- --timeout 5000.

Interesting! I always thought that npm does not pass the extra arguments to pre and post scripts (pretest in this case). I guess I was wrong 🤷

There are debug statements to show how loopback:build is working.

Are you referring to the following line?

https://github.com/strongloop/loopback-next/blob/04728a07e4e22a248f09535d06ec1d3f7323e099/packages/build/bin/compile-package.js#L175

I find the message rather cryptic, how about something like this:

debug('Using the following args for tsc -b', validArgs);

I'm not sure why Travis build fails for some of the acceptance tests - https://github.com/strongloop/loopback-next/pull/5155/checks?check_run_id=603795936.

I quickly checked the error log and don't see any obvious problem 🤔

bajtos

LGTM, but we need to fix CI before this can be landed.

- improve package.json to update project refs and clean *.tsbuildinfo - fix travis script to allow `tsc -b`

In #5155, we reworked our typescript setup to leverage composite mode and project references. This breaks our example applications when they are checked out standalone, typically via `lb4 example`. ```sh $ lb4 example todo (...) $ cd loopback4-example-todo $ npm t (...) > lb-tsc error TS6053: File '/private/packages/http-caching-proxy/tsconfig.json' not found. error TS6053: File '/private/packages/testlab/tsconfig.json' not found. (...) Found 10 errors. ``` This commit fixes the problem by introducing a new step to `lb4 example` command where we remove `references` and `compilerOptions.composite` field from `tsconfig.json` file. Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>

In #5155, we reworked our typescript setup to leverage composite mode and project references. This breaks our example applications when they are checked out standalone, typically via `lb4 example`. ```sh $ lb4 example todo (...) $ cd loopback4-example-todo $ npm t (...) > lb-tsc error TS6053: File '/private/packages/http-caching-proxy/tsconfig.json' not found. error TS6053: File '/private/packages/testlab/tsconfig.json' not found. (...) Found 10 errors. ``` This commit fixes the problem by introducing a new step to `lb4 example` command where we remove `references` field from `tsconfig.json` file and set the field `compilerOptions.composite` to `false`. Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>

raymondfeng requested review from agnes512, bajtos, deepakrkris, dhmlau, emonddr, hacksparrow, jannyHou, marioestradarosa and nabdelgadir as code owners April 19, 2020 20:35

raymondfeng added developer-experience Issues affecting ease of use and overall experience of LB users Internal Tooling Issues related to our tooling and monorepo infrastructore labels Apr 19, 2020

raymondfeng force-pushed the ts-project-refs branch 10 times, most recently from 04fc521 to e90fad4 Compare April 20, 2020 06:09

bajtos suggested changes Apr 20, 2020

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

bin/update-ts-project-refs.js Show resolved Hide resolved

packages/build/bin/compile-package.js Outdated Show resolved Hide resolved

packages/build/config/tsconfig.common.json Outdated Show resolved Hide resolved

raymondfeng force-pushed the ts-project-refs branch from e90fad4 to 6c9a2f5 Compare April 20, 2020 15:53

raymondfeng force-pushed the ts-project-refs branch 2 times, most recently from e5560f2 to 8afd076 Compare April 20, 2020 16:43

raymondfeng force-pushed the ts-project-refs branch from ef10c7f to 04728a0 Compare April 21, 2020 01:37

bajtos reviewed Apr 21, 2020

View reviewed changes

bajtos approved these changes Apr 21, 2020

View reviewed changes

raymondfeng added 5 commits April 21, 2020 14:34

feat(build): update lb-tsc to support -b or --build

bc4e2bb

feat: add a script to update TypeScript project references in tsconfig

07c9420

chore: update tsconfig.json to support project references

5c9c6e8

feat: update package.json and .travis.yml for builds

1fd0a36

- improve package.json to update project refs and clean *.tsbuildinfo - fix travis script to allow `tsc -b`

feat(docs): update docs for TypeScript project refs

649db01

raymondfeng force-pushed the ts-project-refs branch from 04728a0 to 649db01 Compare April 21, 2020 21:34

raymondfeng merged commit 6b24a63 into master Apr 22, 2020

raymondfeng deleted the ts-project-refs branch April 22, 2020 00:06

bajtos mentioned this pull request Apr 23, 2020

fix(cli): remove project references from examples #5204

Merged

3 tasks

This was referenced Apr 23, 2020

Enable TypeScript Project References #2609

Closed

feat: add a script to scaffold and bootstrap a new package #4984

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable TypeScript project references #5155

feat: enable TypeScript project references #5155

raymondfeng commented Apr 19, 2020 •

edited

Loading

bajtos left a comment

raymondfeng commented Apr 20, 2020

raymondfeng commented Apr 20, 2020

raymondfeng commented Apr 20, 2020

bajtos commented Apr 21, 2020

bajtos Apr 21, 2020

bajtos commented Apr 21, 2020

raymondfeng commented Apr 21, 2020

bajtos commented Apr 21, 2020

raymondfeng commented Apr 21, 2020

raymondfeng commented Apr 21, 2020

bajtos commented Apr 21, 2020

bajtos left a comment

feat: enable TypeScript project references #5155

feat: enable TypeScript project references #5155

Conversation

raymondfeng commented Apr 19, 2020 • edited Loading

Checklist

bajtos left a comment

Choose a reason for hiding this comment

raymondfeng commented Apr 20, 2020

raymondfeng commented Apr 20, 2020

raymondfeng commented Apr 20, 2020

bajtos commented Apr 21, 2020

bajtos Apr 21, 2020

Choose a reason for hiding this comment

bajtos commented Apr 21, 2020

raymondfeng commented Apr 21, 2020

bajtos commented Apr 21, 2020

raymondfeng commented Apr 21, 2020

raymondfeng commented Apr 21, 2020

bajtos commented Apr 21, 2020

bajtos left a comment

Choose a reason for hiding this comment

raymondfeng commented Apr 19, 2020 •

edited

Loading