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

Code quality: name store exports #27226

Closed
wants to merge 1 commit into from
Closed

Conversation

adamziel
Copy link
Contributor

@adamziel adamziel commented Nov 24, 2020

Description

#27088 introduced a new recommended way of referencing stores:

Before

select( 'core/viewport' ).isViewportMatch( 'large' ),

After

import { store as viewportStore } from '@wordpress/viewport';
select( viewportStore ).isViewportMatch( 'large' ),

This is great and solves multiple issues we've had. However, I don't like the part that says import { store as viewportStore }. It does not play well with IDE features such as auto importing. Over time, it will likely introduce inconsistencies across the codebase - different files will refer to the same stores using different names. Why not export the preferred name and enjoy "searchability", consistency, and autocompletion? This PR does exactly that:

import { viewportStore } from '@wordpress/viewport';
select( viewportStore ).isViewportMatch( 'large' ),

cc @gziolo @youknowriad @ellatrix

How has this been tested?

Just confirm the units tests passed.

Types of changes

Non-breaking change

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@adamziel adamziel self-assigned this Nov 24, 2020
@adamziel adamziel added [Package] Data /packages/data [Type] Code Quality Issues or PRs that relate to code quality labels Nov 24, 2020
@adamziel adamziel changed the title Named store exports Code quality: name store exports Nov 24, 2020
@github-actions
Copy link

Size Change: +27 B (0%)

Total Size: 1.2 MB

Filename Size Change
build/annotations/index.js 3.8 kB +1 B
build/block-directory/index.js 8.72 kB +1 B
build/block-editor/index.js 133 kB +5 B (0%)
build/blocks/index.js 48.1 kB +1 B
build/core-data/index.js 14.8 kB +3 B (0%)
build/edit-post/index.js 306 kB +2 B (0%)
build/edit-site/index.js 23.6 kB +3 B (0%)
build/edit-widgets/index.js 26.4 kB +2 B (0%)
build/editor/index.js 43.3 kB +4 B (0%)
build/keyboard-shortcuts/index.js 2.55 kB +3 B (0%)
build/notices/index.js 1.82 kB +1 B
build/reusable-blocks/index.js 2.92 kB -1 B
build/rich-text/index.js 13.4 kB +1 B
build/viewport/index.js 1.86 kB +1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/api-fetch/index.js 3.42 kB 0 B
build/autop/index.js 2.84 kB 0 B
build/blob/index.js 664 B 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/style-rtl.css 11.3 kB 0 B
build/block-editor/style.css 11.3 kB 0 B
build/block-library/editor-rtl.css 8.96 kB 0 B
build/block-library/editor.css 8.96 kB 0 B
build/block-library/index.js 148 kB 0 B
build/block-library/style-rtl.css 8.23 kB 0 B
build/block-library/style.css 8.23 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.87 kB 0 B
build/block-serialization-spec-parser/index.js 3.06 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.3 kB 0 B
build/compose/index.js 9.95 kB 0 B
build/data-controls/index.js 827 B 0 B
build/data/index.js 8.8 kB 0 B
build/date/index.js 11.2 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.92 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.45 kB 0 B
build/edit-post/style.css 6.44 kB 0 B
build/edit-site/style-rtl.css 3.86 kB 0 B
build/edit-site/style.css 3.86 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 476 B 0 B
build/editor/editor-styles.css 478 B 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.62 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 6.86 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.27 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 698 B 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.1 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.32 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 790 B 0 B
build/redux-routine/index.js 2.84 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.05 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@gziolo
Copy link
Member

gziolo commented Nov 24, 2020

I'm open to the idea. This is actually how I approached the naming convention in the first place when working on #26655, see a107bb8 :)

I'm fine with what the majority decides.

@youknowriad
Copy link
Contributor

I'm on the fence :) I l ike the consistency of "store" personally but I can see its drawbacks. Waiting to hear more.

@gziolo
Copy link
Member

gziolo commented Nov 24, 2020

I merged 3 PRs that reference store exposed from packages and all of them use aliases when importing. By the way, this PR will need a rebase and some updates to the import statements if we decide to end up using the proposed approach.

@kevin940726
Copy link
Member

I have no strong preference either. I found store consistent, but I can also see that having auto-importing supported in IDE is a great advantage as well.

@draganescu
Copy link
Contributor

I like this as well for reasons of simplicity, as having one preferred name doesn't only result in auto-importing working but also the ease in searching for other places using that particular store (although the same "searchability" is achieved with the string of the package name, this is easier).

@etoledom etoledom removed their request for review December 8, 2020 16:40
Base automatically changed from master to trunk March 1, 2021 15:44
@adamziel adamziel requested a review from getdave as a code owner May 17, 2021 16:09
@dmsnell
Copy link
Member

dmsnell commented Nov 9, 2021

my preference is to keep the aliases for consistency in the modules themselves and for other approaches such as when importing all names from a module

import * as editPost from '@wordpress/edit-post';
import * as viewport from '@wordpress/viewport';

doSomethignWith( editPost.store, viewport.store );

I'm frequently frustrated when working with packages that share a common concept and name but each one makes its own unique version and then I have to try and guess what it is, plus I find that these unique names tend to breakdown anyway and then we deal with the same problem of clashing import names.

@guarani guarani removed their request for review January 19, 2022 13:02
@gziolo gziolo added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Jul 11, 2022
@gziolo
Copy link
Member

gziolo commented Jul 11, 2022

It'll be soon 2 years since this proposal 😅 We didn't take any action so it's rather impossible to change it now because of the backward compatibility concerns. Should we close this PR?

@adamziel adamziel closed this Jul 25, 2022
@gziolo gziolo deleted the try/named-store-exports branch July 25, 2022 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Data /packages/data [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants