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.

Sign up for GitHub

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

Jump to bottom

Determine type of sidebar store automatically #4348

Merged
merged 1 commit into from
Mar 31, 2022
Merged

Determine type of sidebar store automatically #4348

merged 1 commit into from
Mar 31, 2022

Conversation

robertknight
Copy link
Member

@robertknight robertknight commented Mar 24, 2022 •
edited
Loading

Avoid the need to manually create the SidebarStore type by inferring
it automatically. This works as follows:

  1. The modules argument to createStore has been converted to a
    tuple type ([ModuleA, ModuleB, ...])
  2. The type of a module that would result from merging all the
    individual modules (ModuleA & ModuleB ...) is computed
    using a TupleToIntersection utility type
  3. StoreFromModule is used to compute the type of the store that the
    merged module would produce

The merged module described in step 2 is not actually created when the code runs. It is a fiction that is used to "explain" to TS how the output of createStore depends on the input.

@robertknight robertknight force-pushed the inline-create-store-helpers branch from 2e4721d to db54a82 Compare March 24, 2022 13:54
@codecov
Copy link

codecov bot commented Mar 24, 2022 •
edited
Loading

Codecov Report

Merging #4348 (c1f33ab) into master (ac9bf46) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4348   +/-   ##
=======================================
  Coverage   99.17%   99.17%           
=======================================
  Files         225      225           
  Lines        8588     8588           
  Branches     2030     2030           
=======================================
  Hits         8517     8517           
  Misses         71       71
Impacted Files Coverage Δ
src/sidebar/store/create-store.js 100.00% <100.00%> (ø)
src/sidebar/store/index.js 100.00% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@robertknight robertknight force-pushed the inline-create-store-helpers branch from db54a82 to 85d45b9 Compare March 24, 2022 20:04
@robertknight robertknight force-pushed the infer-store-shape branch 3 times, most recently from 8907715 to 7120407 Compare March 25, 2022 07:15
@robertknight robertknight changed the base branch from inline-create-store-helpers to master March 25, 2022 07:15
@robertknight
Determine type of sidebar store automatically
c1f33ab 
Avoid the need to manually create the `SidebarStore` type by inferring
it automatically. This works as follows:

 1. The `modules` argument to `createStore` has been converted to a
    tuple type (`[ModuleA, ModuleB, ...]`)
 2. The type of a module that would result from merging all the
    individual modules (`ModuleA & ModuleB ...`) is computed
    using a `TupleToIntersection` utility type
 3. `StoreFromModule` is used to compute the type of the store that the
    merged module would produce
@robertknight
Copy link
Member Author

robertknight commented Mar 29, 2022 •
edited
Loading

I'm not 100% certain that the trick used here to infer the sidebar store type is a good idea, but we can discuss, and we can easily revert back to the "manual" method of constructing the type if it causes us problems.

robertknight
robertknight commented Mar 29, 2022
View reviewed changes
src/sidebar/store/create-store.js
@@ -149,6 +149,24 @@ function assignOnce(target, source) {
return Object.assign(target, source);
}

/**
* @template T
* @typedef {{[K in keyof T]: (x: T[K]) => void }} MapContravariant
Copy link
Member Author

@robertknight robertknight Mar 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is part of a trick taken from the linked GitHub issue below. I can attempt to explain this, but ah, I don't expect understanding of it. Grokking what the TupleToIntersection utility is used for is enough.

@robertknight robertknight marked this pull request as ready for review March 29, 2022 14:05
lyzadanger
lyzadanger approved these changes Mar 31, 2022
View reviewed changes
Copy link
Contributor

@lyzadanger lyzadanger left a comment

Choose a reason for hiding this comment

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

OK, I think I get this enough to say it looks good :).

src/sidebar/store/create-store.js
@@ -241,7 +260,7 @@ export function createStore(modules, initArgs = [], middleware = []) {
}
Object.assign(store, selectorMethods);

return store;
return /** @type {any} */ (store);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you help me understand this any?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is basically saying "trust me" to TS, since it can't figure out for itself that the object we've put together has the shape we promise.

@robertknight robertknight merged commit 31ac4c8 into master Mar 31, 2022
@robertknight robertknight deleted the infer-store-shape branch March 31, 2022 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lyzadanger lyzadanger lyzadanger approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@robertknight @lyzadanger