Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(shared-object-base): Improve typing in legacy+alpha API surface #23238

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

alexvy86
Copy link
Contributor

@alexvy86 alexvy86 commented Dec 3, 2024

Description

Warning

This is intended to go into the 2.20 release because it includes breaking changes to the legacy+alpha API surface (tracking issue: #23236). Do not merge it before that.

Updates some types in the legacy+alpha API surface of the @fluidframework/shared-object-base package. Mostly improving type safety by replacing any with unknown, and a few any with void where that was the correct type.

Also updates some code in response to the changes above, like call sites of the affected APIs now having to do casts themselves instead of relying on the any they were getting before.

Breaking Changes

The breaking changes affect only the legacy+alpha API surface.

Reviewer Guidance

The review process is outlined on this wiki page.

AB#26129

@alexvy86 alexvy86 requested review from a team as code owners December 3, 2024 17:32
@alexvy86 alexvy86 requested a review from a team December 3, 2024 17:33
@github-actions github-actions bot added area: dds Issues related to distributed data structures area: dds: propertydds area: dds: tree area: tests Tests to add, test infrastructure improvements, etc public api change Changes to a public API base: main PRs targeted against main branch labels Dec 3, 2024
@markfields
Copy link
Member

Curious why there are no type validation test changes needed here?

@alexvy86
Copy link
Contributor Author

Curious why there are no type validation test changes needed here?

Great question, I hadn't thought of that. It seems to be the fact that TS just gives up on any kind of type validation whenever it sees an any. So that's something our type-tests don't seem to be able to protect us from (replacing an any with something else, or viceversa).

I confirmed by adding a small snippet with functions that change their inputs and outputs from unknown to any, and type tests for those don't complain either:

image

Copy link
Contributor

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> [email protected] ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> [email protected] serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> [email protected] check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  170006 links
    1595 destination URLs
    1825 URLs ignored
       0 warnings
       0 errors


@markfields
Copy link
Member

Curious why there are no type validation test changes needed here?

Great question, I hadn't thought of that. It seems to be the fact that TS just gives up on any kind of type validation whenever it sees an any. So that's something our type-tests don't seem to be able to protect us from (replacing an any with something else, or viceversa).

I'm remembering now - last I checked, the type tests' TypeOnly mapped type doesn't try to compare function signatures, so I think your experiment will behave the same with any type there, not just any?

@alexvy86
Copy link
Contributor Author

alexvy86 commented Dec 16, 2024

I'm remembering now - last I checked, the type tests' TypeOnly mapped type doesn't try to compare function signatures, so I think your experiment will behave the same with any type there, not just any?

Yeah, good catch. Example 1 below shows that with a string->number return type change (and the same happens with free functions). That said, it seems it's also true that we don't get protection from any->unknown changes even when they're on props (example 3), where a string->number (example 2), does complain:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dds: propertydds area: dds: tree area: dds Issues related to distributed data structures area: tests Tests to add, test infrastructure improvements, etc base: main PRs targeted against main branch public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants