feat: use composite builds to speed up compilation #692

rflechtner · 2022-11-29T10:38:47Z

fixes KILTProtocol/ticket#2331

In this PR I introduce several optimisations to bring down typescript compile times.

Composite builds and package references. This means that packages that did not change will not be touched on a re-build; therefore, if you change a single file the whole project recompiles in a few seconds. The only downside is that in order to make this work we'll need to maintain references in the tsconfig files of each package that mirrors their dependencies on other packages in the monorepo (as indicated by their package.json).
Only build esm files on full builds: I introduced a build:dev command that is sufficient for testing and development. It only builds cjs and type files.
No typings in esm build: We had type definition files being added to both cjs and esm build folders, which I don't think is necessary; the package.json only points to typings in the cjs folder.

How to test:

Try building on your machine.
Try importing package builds from both esm and cjs code and ts code compiled to cjs and esm.

Checklist:

I have verified that the code works
I have verified that the code is easy to understand
- If not, I have left a well-balanced amount of inline comments
I have left the code in a better state
I have documented the changes (where applicable)

ntn-x2 · 2022-11-29T14:36:22Z

I'll review tomorrow, but I pulled in Tom as well for a review, since I would not rely on my own review for this stuff.

ntn-x2

I like the improvements! I left few comments/questions, but as I said, I would like someone else's opinion on the changes.

ntn-x2 · 2022-11-30T12:47:39Z

package.json

-    "check": "tsc -p tsconfig.json --noEmit",
-    "build": "yarn workspaces foreach -p -t --exclude '{root-workspace}' run build",
+    "check": "tsc -b tsconfig.json",
+    "build": "yarn clean && yarn workspaces foreach -pt --exclude 'root-workspace' run build",


If we run yarn clean before a build, we lose the incremental compilation benefit you mention in the PR description, I guess. So we should probably not clean by default.

This is by design; yarn build is only meant to be run for production builds essentially (it's also the command run in CI). Running a clean beforehand makes sure we don't get any weird artefacts. For standard builds and rebuilds in development you can simply use build:dev.

As mentioned below, we should then somehow advertise that build:dev is the to-go command then.

ntn-x2 · 2022-11-30T12:48:31Z

package.json

+    "check": "tsc -b tsconfig.json",
+    "build": "yarn clean && yarn workspaces foreach -pt --exclude 'root-workspace' run build",
+    "build:dev": "yarn workspaces foreach -pt --exclude 'root-workspace' run build:cjs",
+    "build:watch": "yarn build:dev && tsc -b tsconfig.json --watch",


What's the benefit of this?

It complains as soon as you break something and is more efficient as running yarn check or yarn build:dev repeatedly. Also I would have to double check but potentially you need to have it running if you are using jest in watch mode.

just checked, you would want to have this running as well if jest is running in watch mode to make sure packages are being rebuild on changes. Otherwise imports from another package may use out of date versions.

So this is supposed to be run before and during tests? And yarn build:dev alone then when?

Again, this is for watch mode. As in, you keep jest running and it re-executes tests whenever you change a file. So far we didn't have a solution for that. You can also just run yarn build:dev && yarn test whenever you want to test of course. It's a question of preference.

As for the naming, I was considering making yarn build:dev the new yarn build and introducing a yarn build:production instead for release builds. Do you think that's better? I decided to go the other way because there is no harm in running the production build in a development scenario (it's just slower) but it would be bad to release a development build because the esm files are missing.

This makes sense. Rather use the build for development and something else for production.

ntn-x2 · 2022-11-30T12:49:00Z

package.json

-    "build": "yarn workspaces foreach -p -t --exclude '{root-workspace}' run build",
+    "check": "tsc -b tsconfig.json",
+    "build": "yarn clean && yarn workspaces foreach -pt --exclude 'root-workspace' run build",
+    "build:dev": "yarn workspaces foreach -pt --exclude 'root-workspace' run build:cjs",


I guess this is to be used before unit/integration tests? Can we mention it somewhere? Otherwise I am 99% sure people will still be running yarn build.

True, I haven't updated READMEs and such. Could also add comments above the respective scripts

Okay so we don't have a section on development in the README and we can't add comments to the package.json, where do you think we should put that info?

I guess the README then? I don't see any other place where this could go, personally. Maybe @arty-name has some prior experience for this?

rflechtner · 2022-11-30T17:39:49Z

I'll review tomorrow, but I pulled in Tom as well for a review, since I would not rely on my own review for this stuff.

Agree, would be a great idea to get @arty-name's review on this as well.

arty-name · 2022-12-01T10:16:48Z

I barely have experience with managing monorepos, but I do often see how they invite piling on workarounds and complexity. I am still of the opinion that the SDK should not be a monorepo, and everything will become much simpler and way, way quicker.

Composite builds? Never heard of them, frankly.

ntn-x2 · 2022-12-01T10:20:26Z

@arty-name just out of curiosity, how would a non-monorepo SDK look like? Still all packages in the same repo, I guess? And then each with its own package.json and without a workspace linking them?

arty-name · 2022-12-01T10:29:51Z

Why would they even need their own package.json?

An example: move /packages/core/src/* to /src/core/. That’s it!

arty-name · 2022-12-01T10:32:16Z

An example of increasing complexity: now you need to add extra knowledge to the readme, and there are multiple places that "must be in sync with @KILTprotocol imports in package.json". And then you might be adding in the readme on how to diagnose and solve weird errors that happen when you forget to manually update that thing that must be in sync.

ntn-x2 · 2022-12-01T10:37:22Z

An example: move /packages/core/src/* to /src/core/. That’s it!

I see. So it would become one big package instead of multiple smaller ones, right?

arty-name · 2022-12-01T10:46:39Z

Yes, making something a monorepo is splitting a big package into multiple smaller ones, while removing a monorepo is joining multiple smaller packages into a single bigger one. I have been reporting for quite some time that the SDK’s experiment with a monorepo has failed, in my opinion, and should be reverted.

rflechtner · 2022-12-05T09:55:01Z

I don't really see the end-the-monorepo discussion as related to this change. I'm not saying we shouldn't do it, but it's a different thing. It would completely change the surface of the sdk, while this is just an optimisation of internals.
On the question of added complexity: Essentially, this change introduces a "development build mode" that differs from the current build strategy in two aspects:

Don't clean before build; make sure to rebuild a package only if files changed.
Don't build esm files, you don't need them if you don't publish.

That brings down rebuild times from a whopping 2-3 minutes to less than 10 seconds. This is vital for me to be able to work efficiently.
Everything else is an add-on; if we don't need watch mode or efficient yarn check runs we could also get rid of project references and then we're back to the original complexity, with the difference that I have an extra build command that lets me run tests without a coffee break waiting for the project to build.

arty-name · 2022-12-05T13:44:58Z

I don't really see the end-the-monorepo discussion as related to this change.

According to the PR title, this change is to speed up things, and the "no monorepo" achieves the same goal of 10 seconds. At least I had that a couple of months ago in a quick test.

It would completely change the surface of the sdk

As far as I can remember, using sub-packages was never recommended, and the surface of @kiltprotocol/sdk-js will not change.

rflechtner · 2022-12-05T15:08:11Z

According to the PR title, this change is to speed up things, and the "no monorepo" achieves the same goal of 10 seconds. At least I had that a couple of months ago in a quick test.

That's great, but we simply do not know what would break if we discontinue 10 packages we have published to npm, and 'not having recommended to use them' does not really change anything about that. This is going to take a bit of evaluation and deliberation and is not going to happen tomorrow.
Optimising ts compilation in the current setup has little to do with that, and that's the thing I want to get feedback on. So still think we are sidetracking here.

arty-name · 2022-12-05T16:39:27Z

As I have said in my first comment on this PR #692 (comment) I cannot really contribute here, sorry. I guess the PR achieves its goals 🤷

rflechtner added 10 commits November 29, 2022 00:03

feat: use project references to speed up build

8157873

chore: check & test configs

665a353

fix: add outDir to fix build path

751fb31

fix: point jest to base tsconfig

86a8d24

chore: remove unused jest config directive

c4cc734

fix: linter

9a7744a

chore: prettify

97da75d

chore: remove unused ts directive

8cadee4

fix: exclude tsbuildinfo from package archives

2cd1de6

fix: build scripts

a0bc126

rflechtner requested a review from ntn-x2 November 29, 2022 11:06

rflechtner self-assigned this Nov 29, 2022

rflechtner added the ♻️ refactoring chore: refactoring label Nov 29, 2022

rflechtner added 2 commits November 29, 2022 11:13

fix: bundle tests

88bf269

docs: add explanatory comments to tsconfigs

fba7bb3

ntn-x2 requested a review from arty-name November 29, 2022 14:36

ntn-x2 reviewed Nov 30, 2022

View reviewed changes

chore: make yarn build default (dev) build command

d34e2f7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use composite builds to speed up compilation #692

feat: use composite builds to speed up compilation #692

rflechtner commented Nov 29, 2022 •

edited

Loading

ntn-x2 commented Nov 29, 2022

ntn-x2 left a comment

ntn-x2 Nov 30, 2022

rflechtner Nov 30, 2022 •

edited

Loading

ntn-x2 Dec 1, 2022

ntn-x2 Nov 30, 2022

rflechtner Nov 30, 2022

rflechtner Nov 30, 2022

ntn-x2 Dec 1, 2022

rflechtner Dec 5, 2022

ntn-x2 Dec 12, 2022

ntn-x2 Nov 30, 2022

rflechtner Nov 30, 2022

rflechtner Nov 30, 2022

ntn-x2 Dec 1, 2022

rflechtner commented Nov 30, 2022

arty-name commented Dec 1, 2022

ntn-x2 commented Dec 1, 2022

arty-name commented Dec 1, 2022

arty-name commented Dec 1, 2022

ntn-x2 commented Dec 1, 2022

arty-name commented Dec 1, 2022

rflechtner commented Dec 5, 2022

arty-name commented Dec 5, 2022

rflechtner commented Dec 5, 2022

arty-name commented Dec 5, 2022

feat: use composite builds to speed up compilation #692

Are you sure you want to change the base?

feat: use composite builds to speed up compilation #692

Conversation

rflechtner commented Nov 29, 2022 • edited Loading

fixes KILTProtocol/ticket#2331

How to test:

Checklist:

ntn-x2 commented Nov 29, 2022

ntn-x2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rflechtner Nov 30, 2022 • 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

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

rflechtner commented Nov 30, 2022

arty-name commented Dec 1, 2022

ntn-x2 commented Dec 1, 2022

arty-name commented Dec 1, 2022

arty-name commented Dec 1, 2022

ntn-x2 commented Dec 1, 2022

arty-name commented Dec 1, 2022

rflechtner commented Dec 5, 2022

arty-name commented Dec 5, 2022

rflechtner commented Dec 5, 2022

arty-name commented Dec 5, 2022

rflechtner commented Nov 29, 2022 •

edited

Loading

rflechtner Nov 30, 2022 •

edited

Loading