Packages: Make usage of `core-data` explicit by gziolo · Pull Request #8911 · WordPress/gutenberg

gziolo · 2018-08-13T09:13:50Z

Description

This PR tries to make usage of @wordpress/core-data more explicit. When evaluating #8354 and trying to update all blocks to not have direct API calls I figured out that:

we no longer reference @wordpress/api-fetch in the code but we still list it as dependency
@wordpress/core-data isn't initialized for @wordpress/block-library and @wordpress/editor which might create some issues when using packages outside of WordPress

I also included the change which changes the description of @wordpress/block-library package which we missed last week.

gziolo · 2018-08-13T09:36:34Z

By the way, @wordpress/editor also uses stores from @wordpress/nux and @wordpress/viewport. They are imported and initialized only because we use some components from those packages in some parts of the editor.

youknowriad · 2018-08-13T09:49:05Z

I think we should do that for all stores regardless of whether these modules are already imported for components etc... For example editor should import blocks/viewport, nux and core-data etc...

gziolo · 2018-08-13T10:17:43Z

I asked about that in #8354, I tend to agree with the proposal of making all those imports explicit in the entry file for the package. This way it should be lintable if we agree we want to do it.

gziolo · 2018-08-13T10:42:57Z

When looking at adding explicit dependencies, I discovered that it would produce cyclic dependency between @wordpress/data and @wordpress/core-data:

https://github.com/WordPress/gutenberg/blob/master/packages/data/src/registry.js#L195-L196

gziolo · 2018-08-13T11:07:55Z

When looking at adding explicit dependencies, I discovered that it would produce cyclic dependency between @wordpress/data and @wordpress/core-data:

It turns out that:

select( 'core/data' ) is internal for @wordpress/data
select( 'core' ) is internal for @wordpress/core-data

It is confusing but is what it is. Nothing to worry about :)

aduth · 2018-08-13T13:26:02Z

Will review in more detail shortly, but are these all of the data dependencies we have?

There's also a new merge conflict which needs to be resolved.

gziolo · 2018-08-14T04:59:36Z

I noticed that we should be using "@babel/runtime-corejs2": "7.0.0-beta.56" when rebasing so I included this change, too.

Will review in more detail shortly, but are these all of the data dependencies we have?

I think only editor and block-library use external stores. Otherwise, they are used by the same package which registers them.

youknowriad · 2018-08-15T14:09:33Z

Clearing the milestone because this has no impact on the WordPress scripts.

gziolo · 2018-08-24T05:41:16Z

I rebased it with master, can we move forward with it?

I think the long-term solution is going to be established with #8981. However, it's not a priority at the moment.

youknowriad · 2018-08-24T08:31:32Z

packages/editor/src/index.js

+import '@wordpress/blocks';
+import '@wordpress/core-data';
+import '@wordpress/nux';
+import '@wordpress/viewport';


I think edit-post also uses a bunch of stores, should we add theme there as well?

Added in 2c4f0b0.

youknowriad · 2018-08-24T12:06:38Z

edit-post/index.js

 */
+import '@wordpress/core-data';
+import '@wordpress/editor';
+import '@wordpress/nux';


There is no explicit usage in the module so I missed it ...
I will fix.

We need to figure a more robust way 😄

There is no explicit usage in the module so I missed it ...

This confused me a bit. If there's not explicit usage, the module shouldn't need to define it?

But in looking closer, there is explicit usage:

gutenberg/edit-post/store/effects.js

Line 142 in d7eb4f6

const isMobileViewPort = () => select( 'core/viewport' ).isViewportMatch( '< medium' );

It was all my fault, I didn't grep properly ...

youknowriad

We may have missed some but I'm approving regardless, we can tweak over time until we find a better approach (no global)

aduth · 2018-08-24T14:43:18Z

packages/block-library/package.json

 	"dependencies": {
-		"@babel/runtime": "^7.0.0-beta.52",
-		"@wordpress/api-fetch": "file:../api-fetch",
+		"@babel/runtime-corejs2": "7.0.0-beta.56",


There are local changes to master after npm install with these changes. I think we missed an update to package-lock.json.

I was wondering if there is a way to catch this kind of issue on Travis and fail the build. It isn’t first time when it happens.

gziolo added the npm Packages Related to npm packages label Aug 13, 2018

gziolo added this to the 3.6 milestone Aug 13, 2018

gziolo self-assigned this Aug 13, 2018

gziolo requested a review from youknowriad August 13, 2018 09:13

gziolo mentioned this pull request Aug 13, 2018

wp.data Overview #8354

Closed

6 tasks

gziolo requested a review from aduth August 13, 2018 09:36

gziolo added [Package] Editor /packages/editor [Package] Block library /packages/block-library labels Aug 13, 2018

gziolo force-pushed the update/core-data-usage branch from 35f143b to 19436fa Compare August 14, 2018 04:56

youknowriad removed this from the 3.6 milestone Aug 15, 2018

gziolo added this to the 3.7 milestone Aug 21, 2018

gziolo force-pushed the update/core-data-usage branch from 19436fa to b996fa2 Compare August 24, 2018 05:37

gziolo requested a review from a team August 24, 2018 05:41

youknowriad reviewed Aug 24, 2018

View reviewed changes

gziolo added 3 commits August 24, 2018 12:12

Packages: Make usage of core-data explicit

21e20a4

Editor: Explicitly import all packages which expose used stores

2a128f1

Edit post: Add store imports to ensure other packages are loaded

2c4f0b0

gziolo force-pushed the update/core-data-usage branch from b996fa2 to 2c4f0b0 Compare August 24, 2018 10:19

youknowriad reviewed Aug 24, 2018

View reviewed changes

Add missing viewport import

320365b

youknowriad approved these changes Aug 24, 2018

View reviewed changes

gziolo merged commit 5632f68 into master Aug 24, 2018

gziolo deleted the update/core-data-usage branch August 24, 2018 13:32

aduth reviewed Aug 24, 2018

View reviewed changes

jasmussen mentioned this pull request Aug 27, 2018

Templates: Apply template for new post only #9288

Merged

aduth mentioned this pull request Aug 28, 2018

Framework: Update package-lock.json to expected #9412

Merged

aduth mentioned this pull request Sep 4, 2018

Packages: Add notices package #9617

Merged

Conversation

gziolo commented Aug 13, 2018

Description

Uh oh!

gziolo commented Aug 13, 2018

Uh oh!

youknowriad commented Aug 13, 2018

Uh oh!

gziolo commented Aug 13, 2018

Uh oh!

gziolo commented Aug 13, 2018

Uh oh!

gziolo commented Aug 13, 2018

Uh oh!

aduth commented Aug 13, 2018

Uh oh!

gziolo commented Aug 14, 2018

Uh oh!

youknowriad commented Aug 15, 2018

Uh oh!

gziolo commented Aug 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants