sdk-js icon indicating copy to clipboard operation
sdk-js copied to clipboard

feat: use composite builds to speed up compilation

Open rflechtner opened this issue 2 years ago • 12 comments

fixes KILTProtocol/ticket#2331

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

  1. 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).
  2. 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.
  3. 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.
  • [x] 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)

rflechtner avatar Nov 29 '22 10:11 rflechtner

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 avatar Nov 29 '22 14:11 ntn-x2

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.

rflechtner avatar Nov 30 '22 17:11 rflechtner

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.

arty-name avatar Dec 01 '22 10:12 arty-name

@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?

ntn-x2 avatar Dec 01 '22 10:12 ntn-x2

Why would they even need their own package.json?

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

arty-name avatar Dec 01 '22 10:12 arty-name

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.

arty-name avatar Dec 01 '22 10:12 arty-name

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?

ntn-x2 avatar Dec 01 '22 10:12 ntn-x2

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.

arty-name avatar Dec 01 '22 10:12 arty-name

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:

  1. Don't clean before build; make sure to rebuild a package only if files changed.
  2. 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.

rflechtner avatar Dec 05 '22 09:12 rflechtner

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.

arty-name avatar Dec 05 '22 13:12 arty-name

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.

rflechtner avatar Dec 05 '22 15:12 rflechtner

As I have said in my first comment on this PR https://github.com/KILTprotocol/sdk-js/pull/692#issuecomment-1333534239 I cannot really contribute here, sorry. I guess the PR achieves its goals :shrug:

arty-name avatar Dec 05 '22 16:12 arty-name