Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(build): enable incremental compilation #2928

Merged
merged 1 commit into from
Jun 7, 2019
Merged

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented May 21, 2019

Enable incremental compilation to speed up our builds. Note: this improvement will apply to projects consuming LoopBack too, once they upgrade to the latest version of @loopback/build.

BEFORE

$ time npm run build
  real  0m35.229s
  user  3m51.223s
  sys   0m14.384s

AFTER

The first build to populate tsbuildinfo file:

$ time npm run build
  real  0m35.692s
  user  3m55.728s
  sys   0m13.828s

Subsequent builds (in my case, there were no code changes from the last build):

$ time npm run build
  real  0m19.510s
  user  2m3.894s
  sys   0m9.581s

I am expecting the build times to improve even more when TypeScript 3.5 is released, see https://devblogs.microsoft.com/typescript/announcing-typescript-3-5-rc/

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 bajtos added the Internal Tooling Issues related to our tooling and monorepo infrastructore label May 21, 2019
@bajtos bajtos requested a review from raymondfeng May 21, 2019 09:06
@bajtos bajtos self-assigned this May 21, 2019
@raymondfeng
Copy link
Contributor

Is it extracted from #1636 (I enabled incremental build there too)?

.gitignore Outdated
@@ -13,6 +13,7 @@ benchmark/dist
.sandbox
packages/cli/generators/datasource/connectors.json
docs/site/readmes
**/tsconfig.build.tsbuildinfo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use dist/.tsbuildinfo by configuring tsconfig compilerOptions with:

{
  "tsBuildInfoFile": ".tsbuildinfo"
}

This way, no changes is needed in clean script.

@@ -10,6 +10,8 @@
// FIXME(bajtos) LB4 is not compatible with this setting yet
"strictPropertyInitialization": false,

"incremental": true,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add "tsBuildInfoFile": ".tsbuildinfo",.

Copy link
Member Author

@bajtos bajtos May 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, "tsBuildInfoFile": ".tsbuildinfo" setting in packages/build/config/tsconfig.common.json will be resolved as packages/build/config/.tsbuildinfo, which isn't what we want.

I think we will have to add tsBuildInfoFile setting to every per-package package.json file.

I'll take a look.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, it's resolved against the outDir.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quoting from https://www.typescriptlang.org/docs/handbook/tsconfig-json.html#configuration-inheritance-with-extends:

All relative paths found in the configuration file will be resolved relative to the configuration file they originated in.

See also microsoft/TypeScript#29172

Quoting also from microsoft/TypeScript#29813

  • When tsconfig specifies compiler options incremental file with .tsbuildinfo extension is emitted.
  • Composite projects indirectly specify incremental as true and its error to specify incremental as false in the composite project
  • If project specifies tsBuildInfoFile options, it uses the path specified by that to write the build information.
  • If project specifies out or outFile option, the outFileWithoutExtension.tsbuildinfo file is written next to output js file.
  • If project specified outDir, config files base file name with extension as .tsbuildinfo is written in outDir
  • Otherswise config files base file name with extension as .tsbuildinfo is written next to the config file

AFAICT, we are not setting outDir in our tsconfig files and configure it via a CLI option instead. This is a legacy from the days where we used to have multiple dist directories (dist8 for Node.js 8, dist10 for Node.js 10, etc.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If project specified outDir, config files base file name with extension as .tsbuildinfo is written in outDir

This rule does not seem to be triggered in our current build setup, even when I change all tsconfig.build.json files to include "outDir": "dist" :(

@bajtos
Copy link
Member Author

bajtos commented May 23, 2019

Is it extracted from #1636 (I enabled incremental build there too)?

Not at all, I was not aware of that effort.

In this pull request, I am unconditionally enabling incremental build for all mono-repo packages and all newly created projects. Existing projects created before my changes are not affected.

@bajtos
Copy link
Member Author

bajtos commented May 23, 2019

In this pull request, I am unconditionally enabling incremental build for all mono-repo packages and all newly created projects. Existing projects created before my changes are not affected.

Actually, that's not correct. Because the changes are made in shared tsconfig.common.json, they will apply to existing projects too.

@bajtos
Copy link
Member Author

bajtos commented May 23, 2019

@raymondfeng I have updated the pull request, PTAL again.

  • added outDir and tsBuildInfoFile to per-package tsconfig.build.json files, including CLI templates
  • enabled incremental flag in the shared config

The tests are failing because of a bug in lerna bootstrap, the problem will be fixed by #2944 and lerna/lerna#2104.

@bajtos
Copy link
Member Author

bajtos commented May 30, 2019

added outDir and tsBuildInfoFile to per-package tsconfig.build.json files, including CLI templates

I think we should also modify list of files used by npm pack, so that tsbuildinfo is NOT published to npm.

@raymondfeng
Copy link
Contributor

Maybe we should keep it simple - leave the defaults as is - .tsbuildinfo in the package root directory. We can add .tsbuildinfo to .gitignore.

@bajtos
Copy link
Member Author

bajtos commented Jun 6, 2019

Maybe we should keep it simple - leave the defaults as is - .tsbuildinfo in the package root directory. We can add .tsbuildinfo to .gitignore.

Done.

@raymondfeng LGTY now?

@bajtos bajtos requested a review from raymondfeng June 6, 2019 14:53
.gitignore Show resolved Hide resolved
Copy link
Contributor

@raymondfeng raymondfeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>
@bajtos bajtos requested a review from raymondfeng June 7, 2019 07:04
@bajtos
Copy link
Member Author

bajtos commented Jun 7, 2019

@raymondfeng should we modify npm run clean to remove tsconfig.build.tsbuildinfo files? Should I modify clean script in every per-package package.json file, or should be perhaps improve lb-clean to automatically delete that file?

@bajtos bajtos merged commit 2120712 into master Jun 7, 2019
@delete-merged-branch delete-merged-branch bot deleted the feat/incremental-build branch June 7, 2019 15:09
@bajtos bajtos mentioned this pull request Apr 23, 2020
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal Tooling Issues related to our tooling and monorepo infrastructore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants