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

fix(store-sync): add NoInfer to syncToRecs options #2943

Merged
merged 3 commits into from
Jul 15, 2024

Conversation

alvrs
Copy link
Member

@alvrs alvrs commented Jul 15, 2024

No strong types are needed for this part of the config and @ssalbdivad spotted that the part of the config is a major typescript performance bottleneck.

@alvrs alvrs requested a review from holic as a code owner July 15, 2024 08:58
Copy link

changeset-bot bot commented Jul 15, 2024

🦋 Changeset detected

Latest commit: 926b939

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 23 packages
Name Type
@latticexyz/store-sync Patch
@latticexyz/dev-tools Patch
@latticexyz/store-indexer Patch
@latticexyz/abi-ts Patch
@latticexyz/block-logs-stream Patch
@latticexyz/cli Patch
@latticexyz/common Patch
@latticexyz/config Patch
create-mud Patch
@latticexyz/faucet Patch
@latticexyz/gas-report Patch
@latticexyz/protocol-parser Patch
@latticexyz/query Patch
@latticexyz/react Patch
@latticexyz/recs Patch
@latticexyz/schema-type Patch
solhint-config-mud Patch
solhint-plugin-mud Patch
@latticexyz/store Patch
@latticexyz/utils Patch
@latticexyz/world-modules Patch
@latticexyz/world Patch
mock-game-contracts Patch

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

Copy link
Member

@holic holic left a comment

Choose a reason for hiding this comment

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

Not blocking but do we have or should we have some type tests to make sure this doesn't affect strongly typed components?

@ssalbdivad
Copy link
Collaborator

@alvrs To be clear NoInfer isn't necessarily less safe it's about prioritizing where TS looks when inferring a variable type. My guess is this type is not written as intended in the first place as a structure where the config object both exists as a key and all of its keys exist at the top level of the object is probably not realistic.

It would definitely be valuable to have more tests so we know what might be relying on behavior like this though.

@alvrs
Copy link
Member Author

alvrs commented Jul 15, 2024

a structure where the config object both exists as a key and all of its keys exist at the top level of the object is probably not realistic

SyncOptions includes config (StoreConfig) as a subkey! Here we're adding all general SyncOptions to SyncToRecsOptions. The reason we're doing Omit<..., "config"> & { config } is because config is an optional key in SyncOptions, but a required key in SyncToRecsOptions (maybe there is a more elegant way to perform this modification?)

export type SyncOptions<config extends StoreConfig = StoreConfig> = {
/**
* MUD config
*/
config?: config;

@alvrs alvrs merged commit f640fef into main Jul 15, 2024
13 checks passed
@alvrs alvrs deleted the alvrs/sync-recs-no-infer branch July 15, 2024 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants