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

Monitor store deps in useSelect #26733

Closed
wants to merge 5 commits into from
Closed

Conversation

adamziel
Copy link
Contributor

@adamziel adamziel commented Nov 5, 2020

🚧 TBD

@github-actions
Copy link

github-actions bot commented Nov 5, 2020

Size Change: +576 B (0%)

Total Size: 1.21 MB

Filename Size Change
build/data/index.js 9.34 kB +564 B (6%) 🔍
build/edit-post/index.js 306 kB +12 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.78 kB 0 B
build/api-fetch/index.js 3.45 kB 0 B
build/autop/index.js 2.84 kB 0 B
build/blob/index.js 665 B 0 B
build/block-directory/index.js 8.72 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 132 kB 0 B
build/block-editor/style-rtl.css 11.1 kB 0 B
build/block-editor/style.css 11.1 kB 0 B
build/block-library/editor-rtl.css 9.01 kB 0 B
build/block-library/editor.css 9.01 kB 0 B
build/block-library/index.js 146 kB 0 B
build/block-library/style-rtl.css 7.9 kB 0 B
build/block-library/style.css 7.89 kB 0 B
build/block-library/theme-rtl.css 792 B 0 B
build/block-library/theme.css 793 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 172 kB 0 B
build/components/style-rtl.css 15.3 kB 0 B
build/components/style.css 15.2 kB 0 B
build/compose/index.js 9.81 kB 0 B
build/core-data/index.js 12.5 kB 0 B
build/data-controls/index.js 772 B 0 B
build/date/index.js 31.8 kB 0 B
build/deprecated/index.js 768 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.46 kB 0 B
build/edit-navigation/index.js 11.2 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/style-rtl.css 6.41 kB 0 B
build/edit-post/style.css 6.39 kB 0 B
build/edit-site/index.js 22.5 kB 0 B
build/edit-site/style-rtl.css 3.88 kB 0 B
build/edit-site/style.css 3.88 kB 0 B
build/edit-widgets/index.js 26.3 kB 0 B
build/edit-widgets/style-rtl.css 3.13 kB 0 B
build/edit-widgets/style.css 3.13 kB 0 B
build/editor/editor-styles-rtl.css 480 B 0 B
build/editor/editor-styles.css 482 B 0 B
build/editor/index.js 43.1 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.85 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 7.7 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.16 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 712 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.11 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.34 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.42 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 791 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/reusable-blocks/index.js 3.06 kB 0 B
build/rich-text/index.js 13.2 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@adamziel adamziel force-pushed the try/use-select-monitor-deps branch from 5b706e4 to 7ae23f2 Compare November 5, 2020 20:10
@gziolo gziolo added [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible [Status] In Progress Tracking issues with work in progress labels Nov 7, 2020
@gziolo
Copy link
Member

gziolo commented Nov 8, 2020

@youknowriad, what was the reason createRegistrySelector was introduced in the first place? Is it possible that it is used too often these days?

@gziolo gziolo added the [Package] Data /packages/data label Nov 8, 2020
@youknowriad
Copy link
Contributor

I don't remember exactly the original reasons but I do know we use it for at least two things:

  • we did several refactorings: moving from core/editor to core/block-editor for blocks, moving from core/editor to core/data for entities and with these core/editor selectors need to call selectors from other stores.
  • Initiall isResolving calls were not store specific, we had a common store that tracked resolution state for all the other stores and isResolving from a high-level store were calling the lower level store selectors.

@gziolo
Copy link
Member

gziolo commented Nov 9, 2020

I don't remember exactly the original reasons but I do know we use it for at least two things:

  • we did several refactorings: moving from core/editor to core/block-editor for blocks, moving from core/editor to core/data for entities and with these core/editor selectors need to call selectors from other stores.
  • Initiall isResolving calls were not store specific, we had a common store that tracked resolution state for all the other stores and isResolving from a high-level store were calling the lower level store selectors.

Yes, it all makes sense to use createRegistrySelector for backward compatibility. Do you think, we should refactor all usage of selectors that use this helper as a way to proxy other selectors? My assumption is that if we use createRegistrySelector as a way to proxy deprecations then we don't use them in Gutenberg which makes it possible to find a way to make store specific useStoreSelectors react only to updates scoped to its own store. In fact, we could update the implementation of createRegistrySelector to make it possible for useStoreSelectors to behave like an isolated store until a first call of the selector wrapped with this helper happens. When the createRegistrySelector wrapped selector is called, it would enable backward compatibility mode that listens to updates in all stores.

@youknowriad
Copy link
Contributor

Do you think, we should refactor all usage of selectors that use this helper as a way to proxy other selectors?

What do you mean? these are already proxy selectors.

I also think we have use-cases where we want to combine results from several stores in a single selector.

@adamziel
Copy link
Contributor Author

adamziel commented Nov 9, 2020

I also think we have use-cases where we want to combine results from several stores in a single selector.

Couldn't those be replaced with editor-specific react hooks? This way they wouldn't have to be exposed as a public API by default. And no registry selectors = less overall complexity.

@gziolo
Copy link
Member

gziolo commented Nov 9, 2020

Do you think, we should refactor all usage of selectors that use this helper as a way to proxy other selectors?

What do you mean? these are already proxy selectors.

There are more selectors like the following:

export const getCanUserCreateMedia = createRegistrySelector( ( select ) => () =>
select( 'core' ).canUser( 'create', 'media' )
);

@adamziel adamziel mentioned this pull request Nov 9, 2020
6 tasks
@adamziel
Copy link
Contributor Author

adamziel commented Nov 9, 2020

I added some relevant considerations in #26692 (comment) - let's move the discussion there?

@adamziel
Copy link
Contributor Author

adamziel commented Nov 9, 2020

Closing for now (see #26692 (comment)).

@adamziel adamziel closed this Nov 9, 2020
@youknowriad youknowriad deleted the try/use-select-monitor-deps branch November 9, 2020 17:45
@gziolo gziolo mentioned this pull request Nov 13, 2020
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Data /packages/data [Status] In Progress Tracking issues with work in progress [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants