Edit Post: Replace store name string with exposed store definition #27414

rafaelgallani · 2020-12-01T21:18:51Z

Description

Addresses #27088. Replaces the store names (hardcoded strings) with the exposed store definitions.

How has this been tested?

npm run test and npm run test-e2e, making sure the tests didn't break.

Types of changes

Code refactoring, 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.

rafaelgallani · 2020-12-01T21:35:42Z

There are still some references remaining, but most of them are in the e2e-tests and e2e-tests-utils packages. What should I do in these cases? Adding edit-post as a dependency doesn't look right.

There's also selectors.js in edit-post package, which I didn't change since it's not in a select/useSelect/withSelect/dispatch/useDispatch/withDispatch call. Should I change it anyway?

rafaelgallani

Some stable tests (according to the last runs in the master branch) were failing. I reviewed and found some copy/paste mistakes. I'll fix them and see if it fixes the failing tests.

packages/edit-post/src/components/sidebar/settings-sidebar/index.js

packages/edit-post/src/store/actions.js

rafaelgallani · 2020-12-01T21:58:22Z

In this case, how should I perform these changes? Should I rebase my branch or should I add one more commit on top of it?
I would prefer rebasing + force pushing to keep the commit history clean. I couldn't find recommendations for this situation in the git guidelines.

packages/edit-post/src/components/layout/index.js

rafaelgallani · 2020-12-02T14:36:26Z

Rebasing on upstream/master.

rafaelgallani · 2020-12-02T17:59:51Z

At first, the build failed because I forgot to include @wordpress/edit-post in the package.json of each changed package.
I thought that because of this:

To be clear, all packages that use a given datastore should import it in the entry point by convention. It's something that could be improved because if it's missed, it might indeed cause errors. At the moment, there is no simple way to validate it happens. Example:

gutenberg/packages/edit-post/src/index.js

Lines 4 to 6 in 87fa5f5

import '@wordpress/core-data';

import '@wordpress/block-editor';

import '@wordpress/editor';

They already declared it... But anyway. That is fixed now.
The problem now looks like a cyclic dependency problem... Though I'm not sure of it. That is what I could understand by looking at the stack in the action's log:

FAIL packages/block-library/src/list/test/edit.native.js
  ● Test suite failed to run

    TypeError: Cannot read property 'SETTINGS_DEFAULTS' of undefined



 ---> at Object.SETTINGS_DEFAULTS (packages/block-editor/src/index.js:24:22)
      at Object.<anonymous> (packages/editor/src/store/defaults.js:25:5)
      at Object.<anonymous> (packages/editor/src/store/reducer.js:15:1)
      at Object.<anonymous> (packages/editor/src/store/reducer.native.js:14:1)
      at Object.<anonymous> (packages/editor/src/store/index.js:10:1)
      at Object.<anonymous> (packages/editor/src/index.native.js:13:1)
      at Object.<anonymous> (packages/edit-post/src/editor.native.js:12:1)
      at Object.<anonymous> (packages/edit-post/src/index.native.js:13:1)
      at Object.<anonymous> (packages/components/src/mobile/link-settings/index.native.js:10:1)
      at Object.<anonymous> (packages/components/src/index.native.js:78:1)
      at Object.<anonymous> (packages/rich-text/src/component/index.native.js:20:1)
      at Object.<anonymous> (packages/rich-text/src/index.js:37:1)
 ---> at Object.<anonymous> (packages/block-editor/src/index.js:4:1)   
      at Object.<anonymous> (packages/block-library/src/list/edit.js:6:1)
      at Object.<anonymous> (packages/block-library/src/list/test/edit.native.js:9:1)

@gziolo Is my understanding is correct? If yes, then is there any example where this has been fixed before, or have I created a monster? 😞 Also, sorry for writing so much; I wanted to make myself clear as I thought this very confusing to explain.

gziolo · 2020-12-02T18:21:15Z

Yes, those cyclic dependencies are bugs. That's the point of this refactoring to fix them eventually and prevent moving forward 😃

I plan to look at your PR tomorrow, great comments and awesome work 🎉

rafaelgallani · 2020-12-02T18:27:55Z

Yes, those cyclic dependencies are bugs. That's the point of this refactoring to fix them eventually and prevent moving forward

I plan to look at your PR tomorrow, great comments and awesome work

Great, thanks! I'll keep looking at it and see if I can come up with anything else.

gziolo · 2020-12-03T13:07:42Z

@hypest and @Tug, it looks like there are some architectural issues in React Native files that this PR uncovers. The packages should depend on each other in the following direction:

edit-post
- block-library
  - editor
    - blocks
    - components
      - element

There is also some mix in the picture as edit-post itself might be using one of the deeper dependencies directly, and so on.

@rafaelgalani, what it means is that for now, we should live hardcoded strings outside of the @wordpress/edit-post package and fix all the issues separately. It might feel like you wasted some time, but in reality, you helped to identify potential bugs. For now, let's revert changes that create cyclic dependencies and use the revert commit as a baseline to the follow-up issue that I can create myself later.

There are other high-level packages: edit-site, edit-widgets, or edit-navigation that shouldn't be referenced externally as well. I probably should include a note in the parent issue as well.

block-directory is a snowflake here. It's an experiment that can depend on edit-post as it works as an extension to the editor so it should be fine to leave modified. I can test that once the branch is testable.

gziolo · 2020-12-03T13:23:57Z

I missed some previous questions:

There are still some references remaining, but most of them are in the e2e-tests and e2e-tests-utils packages. What should I do in these cases? Adding edit-post as a dependency doesn't look right.

Yes, for all code related to e2e tests we should leave hard-coded strings, those are shortcuts that allow to perform more detailed assertions or set the app in a specific state.

There's also selectors.js in edit-post package, which I didn't change since it's not in a select/useSelect/withSelect/dispatch/useDispatch/withDispatch call. Should I change it anyway?

They should be left unmodified. Those are hardcoded strings used for scoping which happen to use the same names to make it easier to debug.

Should I rebase my branch or should I add one more commit on top of it?
I would prefer rebasing + force pushing to keep the commit history clean. I couldn't find recommendations for this situation in the git guidelines.

It's up to you. I personally use rebasing + force pushing, sometimes I squash commits. I think it's covered here: https://github.com/WordPress/gutenberg/blob/master/docs/contributors/git-workflow.md#keeping-your-branch-up-to-date. It's for repository members (it's you now, you can open PR directly in Gutenberg repo), there is also a section that covers forks (this branch).

rafaelgallani · 2020-12-04T02:20:45Z

Uhm, @gziolo...

gutenberg/packages/edit-post/src/index.js

Line 19 in 2d24e16

import './store';

Sorry. I haven't checked the exports... They were included in the last refactorings, so I thought that for this package they would be too, my bad. 😓
I'll add the export and check the builds to see what could be breaking again - aka want to ensure that there is really a cyclic dependency problem and not something caused by the missing exports. At least I'm getting the hang of working on the project, lol.

Reverting commits that removed `"edit-post"` hardcoded store name on `"components"` and `"block-editor"` packages, due to cyclic dependencies, as referenced in WordPress#27414 (this commit will be used as baseline later).

rafaelgallani · 2020-12-04T20:31:05Z

I only force-pushed to resolve the conflicts. There is no actual change to the code.

packages/edit-post/README.md

gziolo

I left two more comments. Once they are addressed this PR should be good to 😄

Edit: I posted this comment while doing a live stream at Twitch 😅:
https://www.twitch.tv/videos/828801176

gziolo · 2020-12-07T13:06:48Z

packages/editor/src/components/provider/index.native.js

@@ -28,6 +28,7 @@ import {
 	createBlock,
 } from '@wordpress/blocks';
 import { withDispatch, withSelect } from '@wordpress/data';
+import { store as editPostStore } from '@wordpress/edit-post';


Let's avoid this cyclic dependency for @wordpress/editor package and leave it as a string for now.

gziolo · 2020-12-07T13:07:16Z

packages/block-library/package.json

@@ -40,6 +40,7 @@
 		"@wordpress/date": "file:../date",
 		"@wordpress/deprecated": "file:../deprecated",
 		"@wordpress/dom": "file:../dom",
+		"@wordpress/edit-post": "file:../edit-post",


Let's avoid this cyclic dependency for @wordpress/block-library package and leave it as a string for now.

rafaelgallani · 2020-12-07T21:01:02Z

I left two more comments. Once they are addressed this PR should be good to

Edit: I posted this comment while doing a live stream at Twitch :
https://www.twitch.tv/videos/828801176

+1 follow 😆. Will fix them right away.

hypest · 2020-12-08T15:22:08Z

There is this issue though that codebase is tightly coupled to the specific screen - edit post. As soon as you will want to support another screens for things like a site, theme templates or maybe post comments, it will become a blocker. We don't have a solid solution for that, so far we were using props or editor settings for passing down information where necessary. It feels like we need some sort of scope detection to make it more robust. If you have some ideas, I'm happy to hear your thoughts.

👋 @gziolo, can you open a ticket to have that discussion? I feel like there must be some immediate changes we could do to adapt the native mobile blocks to use whatever is the pattern the web components have atm, but I'm not sure what that pattern is. I like how you opened #27088 with a checklist of the blocks that need refactoring and would be nice if we compile a similar one for the cases you bring up here. WDYT?

…-definition

packages/block-library/package.json

packages/block-library/src/button/edit.native.js

packages/block-library/src/columns/edit.native.js

packages/block-library/src/file/edit.native.js

packages/block-library/src/latest-posts/edit.native.js

packages/editor/package.json

packages/editor/src/components/provider/index.native.js

gziolo · 2020-12-16T07:10:19Z

package-lock.json

@@ -13247,6 +13247,7 @@
 				"@wordpress/date": "file:packages/date",
 				"@wordpress/deprecated": "file:packages/deprecated",
 				"@wordpress/dom": "file:packages/dom",
+				"@wordpress/edit-post": "file:packages/edit-post",


Suggested change

"@wordpress/edit-post": "file:packages/edit-post",

gziolo · 2020-12-16T07:12:18Z

I reverted changes to @wordpress/editor and @wordpress/block-library packages. I contemplated on using export for store on @wordpress/edit-post and I think you were right, it makes sense for 3rd party plugin. @wordpress/block-directory is an internal plugin so it makes sense to keep it working this way.

I can't apply changes to the lock file using GitHub UI. Would you mind running npm install locally to regenerate the lock file? Other than that this PR should be ready to go 🎉

gziolo · 2020-12-16T07:28:11Z

I opened #27751 to address the issue with cyclic dependencies in other packages - those reverted in this PR.

gziolo

@rafaelgalani thank you for your great cooperation on this PR.

rafaelgallani commented Dec 1, 2020

View reviewed changes

packages/edit-post/src/components/sidebar/settings-sidebar/index.js Outdated Show resolved Hide resolved

packages/edit-post/src/store/actions.js Outdated Show resolved Hide resolved

gziolo added [Type] Code Quality Issues or PRs that relate to code quality [Package] Edit Post /packages/edit-post labels Dec 1, 2020

rafaelgallani commented Dec 2, 2020

View reviewed changes

packages/edit-post/src/components/layout/index.js Outdated Show resolved Hide resolved

rafaelgallani force-pushed the refactor/replace-edit-post-store-name-with-definition branch from 45c0692 to bda42e4 Compare December 2, 2020 03:27

gziolo mentioned this pull request Dec 2, 2020

Code quality: Replace hardcoded store keys with store definitions #27088

Closed

17 tasks

rafaelgallani force-pushed the refactor/replace-edit-post-store-name-with-definition branch from bda42e4 to a2bb533 Compare December 2, 2020 14:36

rafaelgallani marked this pull request as draft December 2, 2020 14:58

rafaelgallani force-pushed the refactor/replace-edit-post-store-name-with-definition branch from a2bb533 to e728d8c Compare December 2, 2020 17:07

gziolo added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Dec 3, 2020

rafaelgallani marked this pull request as ready for review December 4, 2020 00:33

rafaelgallani requested review from ajitbohra and talldan as code owners December 4, 2020 00:33

rafaelgallani force-pushed the refactor/replace-edit-post-store-name-with-definition branch from ab78919 to e9fac2b Compare December 4, 2020 00:48

rafaelgallani force-pushed the refactor/replace-edit-post-store-name-with-definition branch from e9fac2b to f2a297c Compare December 4, 2020 02:21

rafaelgallani requested review from chrisvanpatten and ellatrix as code owners December 4, 2020 02:21

rafaelgallani added 4 commits December 4, 2020 17:15

Removing 'components' references to the store name

f87a81a

Removing 'edit-post' references to the store name

1241c20

Removing 'editor' references to the store name

385dcef

Reverting changes that caused cyclic dependencies

58ab3d1

Reverting commits that removed `"edit-post"` hardcoded store name on `"components"` and `"block-editor"` packages, due to cyclic dependencies, as referenced in WordPress#27414 (this commit will be used as baseline later).

rafaelgallani force-pushed the refactor/replace-edit-post-store-name-with-definition branch from af20ba5 to 58ab3d1 Compare December 4, 2020 20:15

gziolo reviewed Dec 7, 2020

View reviewed changes

packages/edit-post/README.md Show resolved Hide resolved

gziolo reviewed Dec 7, 2020

View reviewed changes

Merge branch 'master' into refactor/replace-edit-post-store-name-with…

de6585c

…-definition

gziolo reviewed Dec 16, 2020

View reviewed changes

gziolo added 6 commits December 16, 2020 07:52

Apply suggestions from code review

73792a3

Apply suggestions from code review

8450163

Apply suggestions from code review

1fcee4c

Apply suggestions from code review

4b34993

Apply suggestions from code review

ab787bc

Update packages/block-library/package.json

ba0da5f

gziolo reviewed Dec 16, 2020

View reviewed changes

gziolo mentioned this pull request Dec 16, 2020

Mobile App: Avoid code references that create cyclic dependencies #27751

Open

4 tasks

Apply suggestions from code review

19222ce

rafaelgallani force-pushed the refactor/replace-edit-post-store-name-with-definition branch from 05b7417 to 19222ce Compare December 16, 2020 21:28

gziolo approved these changes Dec 17, 2020

View reviewed changes

gziolo merged commit c88866c into WordPress:master Dec 17, 2020

github-actions bot added this to the Gutenberg 9.7 milestone Dec 17, 2020

rafaelgallani mentioned this pull request Jan 13, 2021

Annotations: Replace store name string with exposed store definition #28156

Merged

6 tasks

hypest mentioned this pull request Apr 14, 2021

[RNMobile] Add Featured Banner to Image Block (Android Only) #30806

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Edit Post: Replace store name string with exposed store definition #27414

Edit Post: Replace store name string with exposed store definition #27414

rafaelgallani commented Dec 1, 2020 •

edited

Loading

rafaelgallani commented Dec 1, 2020

rafaelgallani left a comment

rafaelgallani commented Dec 1, 2020

rafaelgallani commented Dec 2, 2020

rafaelgallani commented Dec 2, 2020 •

edited

Loading

gziolo commented Dec 2, 2020 •

edited

Loading

rafaelgallani commented Dec 2, 2020

gziolo commented Dec 3, 2020 •

edited

Loading

gziolo commented Dec 3, 2020

rafaelgallani commented Dec 4, 2020 •

edited

Loading

rafaelgallani commented Dec 4, 2020

gziolo left a comment •

edited

Loading

gziolo Dec 7, 2020

gziolo Dec 7, 2020

rafaelgallani commented Dec 7, 2020

hypest commented Dec 8, 2020

gziolo Dec 16, 2020

gziolo commented Dec 16, 2020

gziolo commented Dec 16, 2020

gziolo left a comment

Edit Post: Replace store name string with exposed store definition #27414

Edit Post: Replace store name string with exposed store definition #27414

Conversation

rafaelgallani commented Dec 1, 2020 • edited Loading

Description

How has this been tested?

Types of changes

Checklist:

rafaelgallani commented Dec 1, 2020

rafaelgallani left a comment

Choose a reason for hiding this comment

rafaelgallani commented Dec 1, 2020

rafaelgallani commented Dec 2, 2020

rafaelgallani commented Dec 2, 2020 • edited Loading

gziolo commented Dec 2, 2020 • edited Loading

rafaelgallani commented Dec 2, 2020

gziolo commented Dec 3, 2020 • edited Loading

gziolo commented Dec 3, 2020

rafaelgallani commented Dec 4, 2020 • edited Loading

rafaelgallani commented Dec 4, 2020

gziolo left a comment • edited Loading

Choose a reason for hiding this comment

gziolo Dec 7, 2020

Choose a reason for hiding this comment

gziolo Dec 7, 2020

Choose a reason for hiding this comment

rafaelgallani commented Dec 7, 2020

hypest commented Dec 8, 2020

gziolo Dec 16, 2020

Choose a reason for hiding this comment

gziolo commented Dec 16, 2020

gziolo commented Dec 16, 2020

gziolo left a comment

Choose a reason for hiding this comment

rafaelgallani commented Dec 1, 2020 •

edited

Loading

rafaelgallani commented Dec 2, 2020 •

edited

Loading

gziolo commented Dec 2, 2020 •

edited

Loading

gziolo commented Dec 3, 2020 •

edited

Loading

rafaelgallani commented Dec 4, 2020 •

edited

Loading

gziolo left a comment •

edited

Loading