Skip to content

Enable typechecking in CI for base-data-service - #10009

Open
mcmire wants to merge 1 commit into
mainfrom
enable-typechecking-for-base-data-service
Open

Enable typechecking in CI for base-data-service#10009
mcmire wants to merge 1 commit into
mainfrom
enable-typechecking-for-base-data-service

Conversation

@mcmire

@mcmire mcmire commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Explanation

To ensure all files in base-data-service — not just production files — are typechecked in CI now and in the future:

  • Add tsconfig.lint.json for base-data-service and hook it into root tsconfig.lint.json
  • Fix type errors in tests
    • Add missing StorageService:* actions to ExampleDataService messenger
    • Refactor test setup by adding createRootMessenger and createServiceMessenger helpers

References

(N/A)

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've communicated my changes to consumers by updating changelogs for packages I've changed
  • I've introduced breaking changes in this PR and have prepared draft pull requests for clients and consumer packages to resolve them

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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?

@mcmire

mcmire commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator Author

Adding no-changelog since there are no changes to source files.

@mcmire mcmire changed the title Enable typechecking for BaseDataService Enable typechecking in CI for BaseDataService Aug 28, 2026
@mcmire
mcmire marked this pull request as ready for review August 28, 2026 19:16
@mcmire
mcmire requested a review from a team as a code owner August 28, 2026 19:16
@mcmire
mcmire deployed to default-branch August 28, 2026 19:17 — with GitHub Actions Active
@mcmire mcmire changed the title Enable typechecking in CI for BaseDataService Enable typechecking in CI for base-data-service Aug 28, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ 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();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit da55e45. Configure here.

function createServiceMessenger(
rootMessenger = createRootMessenger(),
): ExampleMessenger {
const messenger: ExampleMessenger = new Messenger({

@mcmire mcmire Aug 28, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants