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 10 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

@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

Copy link
Contributor

github-actions bot commented Jan 7, 2025

🔗 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:
  170492 links
    1603 destination URLs
    1838 URLs ignored
       0 warnings
       0 errors


@alexvy86
Copy link
Contributor Author

alexvy86 commented Jan 8, 2025

@Josmithr @markfields any further comments here? The integration pipeline with Loop passed so it should be good to merge.

@Josmithr Josmithr self-requested a review January 8, 2025 01:21
Copy link
Contributor

@Josmithr Josmithr left a comment

Choose a reason for hiding this comment

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

It would be good to add a changeset for this, but otherwise looks good to me!

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.

Replace any with unknown and fix an any->void return type in the shared-object-base package
3 participants