[v2] Мonorepo, sdk-client, sdk-server, sdk-core package split
#1279
Conversation
commit: |
… into feature/v2-monorepo-setup
|
EDITs:
TODOs still:
|
… into feature/v2-monorepo-setup
|
I would push against removing tsgo from the check script. As we move to a monorepo it will be much much faster than tsc. |
| "node": ">=20", | ||
| "pnpm": ">=10.24.0" | ||
| }, | ||
| "packageManager": "[email protected]", |
There was a problem hiding this comment.
I dont hate this move. I like that we wont have to use turborepo or nx, at least initially. But it should be noted this will be a big dev ex change
There was a problem hiding this comment.
Yes, we would need to update the README and the contribution guidelines
| export * from './validation/ajv-provider.js'; | ||
| export * from './validation/cfworker-provider.js'; |
There was a problem hiding this comment.
these have to be exported in the package.json. otherwise they will both get included on import.
There was a problem hiding this comment.
really we should use shims for this in the package.json and not have separate modules. similarly to this. mswjs/msw#2640
There was a problem hiding this comment.
also feels pretty weird that this is a package. imo these tests should live in either client or server depending on what they are testing.
There was a problem hiding this comment.
you could have some test helpers but they would live in common.
There was a problem hiding this comment.
Yes, this one felt weird to me as well and haven't spent a lot of time thinking about it.
Let's think how we want it and refactor in a separate PR.
We could bring it back, but I did experience some issues with it. In theory it should work with typescript v5, but there are some weird stuff going on. FWIW, did not feel the speed diff on this big of a codebase, but let's bring it back in another PR if we want it |
🦋 Changeset detectedLatest commit: 60de202 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Awesome refactor, looking great. THANK YOU for taking the time to ensure git history is preserved - that made it MUCH easier to review 95% of this PR as the vast majority of this PR is just shuffling around imports for the new structure.
I peppered with comments, but most of them are questions I had as I was working through so I don't forget - not asks for modifications. Please ignore comments except this one for the next hour or so while I investigate and try to answer my own questions.
Some things I wanted to discuss for sure though:
-
We use!type checker overrides a lot in this PR. Is this an indication of behavior change or we've somehow tightenedtsconfigso we have to do this to satisfy the linter and these are pre-existing problems?
Addressed, we tightened our type checks and this has no runtime effect. If anything it's good to make it explicit so we can periodically fix it. -
I think we missed one test file
test/experimental/tasks/stores/in-memory.test.ts- there are 59 tests less in this version than in the currentmain, and this file contains exactly 59 tests. -
We're creating a lot of packages andpackage.jsonfiles - my expectation was that we'd have 3 max:client,server,core. Is this typical? Are we adding complexity for either users or ourselves by doing this?
Addressed, welcome to Typescript 2025. Minimize dependencies we force people to import. -
Why change topnpm? What's the advantage of this? Are we making it more difficult for people to install the SDK? Is this a DevX improvement? I have no intuition of what this change means, is this a drop in replacement fornpm?
Addressed, this provides us a lot of utilities for monorepo dev with multiple packages thatnpmdoesn't support. It's also included withcorepackvia Node anyway, so minimal Dev friction for contributors, zero change for consumers of the SDK. -
Did you (or some agent 😄) potentially verify that the examples all still work as written here? Or is that something we still need to do?
| import { TaskCreationParams, Request } from '../../../../src/types.js'; | ||
| import { QueuedMessage } from '../../../../src/experimental/tasks/interfaces.js'; | ||
|
|
||
| describe('InMemoryTaskStore', () => { |
There was a problem hiding this comment.
did we miss this set of tests in the refactor?
There was a problem hiding this comment.
I think we missed this test - Ran a comparison of all tests before and after:
Before: 1495 tests total
After: 1436 tests total
This file has 59 tests by my count:
npx vitest run test/experimental/tasks/stores/in-memory.test.ts
...
Test Files 1 passed (1)
Tests 59 passed (59)
Start at 17:42:14
Duration 140ms (transform 39ms, setup 14ms, collect 37ms, tests 9ms, environment 0ms, prepare 5ms)
There was a problem hiding this comment.
We should probably bring this back
|
Noting down some follow-ups to this PR we'll want to do as a note to self:
|
Proof of Concept: v2 monorepo, package split
Motivation and Context
v1of the sdk shipped both client and server in the same package, introducing extra dependencies to users who have a single concern (client or server only). Client and server packages are now split, depending on a sharedcorepackage.Additionally,
v1exported all files as the public API of the SDK, limiting the SDK ability to iterate internally while keeping backwards compatibility. Barrel files are now introduced as the single entrypoint to the SDK and defining the public API of the SDK.How Has This Been Tested?
Unit, integration tests
Breaking Changes
v2
Types of changes
Checklist
Additional context