Enable typechecking in CI for base-data-service - #10009
Conversation
To ensure test files in `base-data-service` are typechecked in the future: - Add `tsconfig.lint.json` for package and hook into root `tsconfig.lint.json` - Fix type errors in tests and refactor setup code
| | ExampleDataServiceMethodActions | ||
| | DataServiceInvalidateQueriesAction<typeof serviceName>; | ||
| | DataServiceInvalidateQueriesAction<typeof serviceName> | ||
| | StorageServiceGetItemAction |
There was a problem hiding this comment.
I found that this was necessary in order to remove the type errors above. It seems that BaseDataService does not verify that these actions are present on the messenger that it expects. Is that a bug?
|
Adding |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit da55e45. Configure here.
| expect.anything(), | ||
| expect.anything(), | ||
| ); | ||
| expect(setItem).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
Rehydration test asserts wrong storage action
Medium Severity
The skips rehydrating cache if persistConfig is not set test registers and asserts StorageService:setItem, but rehydration goes through StorageService:getItem. The previous version asserted getItem was not called. As written, the test still passes if init rehydrates without persistConfig, so it no longer guards that path.
Reviewed by Cursor Bugbot for commit da55e45. Configure here.
| function createServiceMessenger( | ||
| rootMessenger = createRootMessenger(), | ||
| ): ExampleMessenger { | ||
| const messenger: ExampleMessenger = new Messenger({ |
There was a problem hiding this comment.
I opted not to use buildChild in this refactor because it doesn't seem to assign the right types by default: it will give the resulting messenger a type that includes the actions that we are delegating, but we actually need the resulting type to include ExampleDataService capability types too.
Besides, adding a type annotation like this to "force" the type is consistent with how we generally create root messengers (see createRootMessenger above). Technically we still could use buildChild in this case and get away with it, but that would work accidentally (because in specifying a return type we are actually assigning a type annotation to this function, so we're still "forcing" TypeScript to do what we want). But again, I like the consistency as it's easier for me to keep straight.


Explanation
To ensure all files in
base-data-service— not just production files — are typechecked in CI now and in the future:tsconfig.lint.jsonforbase-data-serviceand hook it into roottsconfig.lint.jsonStorageService:*actions toExampleDataServicemessengercreateRootMessengerandcreateServiceMessengerhelpersReferences
(N/A)
Checklist