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

Packages: Make usage of `core-data` explicit #8911

Merged
merged 4 commits into from Aug 24, 2018

Conversation

Projects
None yet
3 participants
@gziolo
Member

gziolo commented Aug 13, 2018

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 gziolo added the Packages label Aug 13, 2018

@gziolo gziolo added this to the 3.6 milestone Aug 13, 2018

@gziolo gziolo self-assigned this Aug 13, 2018

@gziolo gziolo requested a review from youknowriad Aug 13, 2018

@gziolo gziolo referenced this pull request Aug 13, 2018

Open

wp.data Overview #8354

3 of 5 tasks complete
@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Aug 13, 2018

Member

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.

Member

gziolo commented Aug 13, 2018

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

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 13, 2018

Contributor

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...

Contributor

youknowriad commented Aug 13, 2018

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

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Aug 13, 2018

Member

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.

Member

gziolo commented Aug 13, 2018

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

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Aug 13, 2018

Member

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

Member

gziolo commented Aug 13, 2018

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

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Aug 13, 2018

Member

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 :)

Member

gziolo commented Aug 13, 2018

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

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Aug 13, 2018

Member

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.

Member

aduth commented Aug 13, 2018

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

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Aug 14, 2018

Member

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.

Member

gziolo commented Aug 14, 2018

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 youknowriad removed this from the 3.6 milestone Aug 15, 2018

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 15, 2018

Contributor

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

Contributor

youknowriad commented Aug 15, 2018

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

@gziolo gziolo added this to the 3.7 milestone Aug 21, 2018

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Aug 24, 2018

Member

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.

Member

gziolo commented Aug 24, 2018

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.

@gziolo gziolo requested a review from WordPress/gutenberg-core Aug 24, 2018

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

This comment has been minimized.

@youknowriad

youknowriad Aug 24, 2018

Contributor

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

@youknowriad

youknowriad Aug 24, 2018

Contributor

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

This comment has been minimized.

@gziolo

gziolo Aug 24, 2018

Member

Added in 2c4f0b0.

@gziolo

gziolo Aug 24, 2018

Member

Added in 2c4f0b0.

@@ -1,6 +1,9 @@
/**
* WordPress dependencies
*/
import '@wordpress/core-data';
import '@wordpress/editor';
import '@wordpress/nux';

This comment has been minimized.

@youknowriad

youknowriad Aug 24, 2018

Contributor

viewport?

@youknowriad

youknowriad Aug 24, 2018

Contributor

viewport?

This comment has been minimized.

@gziolo

gziolo Aug 24, 2018

Member

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

We need to figure a more robust way 😄

@gziolo

gziolo Aug 24, 2018

Member

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

We need to figure a more robust way 😄

This comment has been minimized.

@aduth

aduth Aug 24, 2018

Member

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:

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

@aduth

aduth Aug 24, 2018

Member

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:

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

This comment has been minimized.

@gziolo

gziolo Aug 24, 2018

Member

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

@gziolo

gziolo Aug 24, 2018

Member

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)

@gziolo gziolo merged commit 5632f68 into master Aug 24, 2018

2 checks passed

codecov/project 50.59% (-0.02%) compared to 26a87d9
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gziolo gziolo deleted the update/core-data-usage branch Aug 24, 2018

@@ -20,8 +20,7 @@
"module": "build-module/index.js",
"react-native": "src/index",
"dependencies": {
"@babel/runtime": "^7.0.0-beta.52",
"@wordpress/api-fetch": "file:../api-fetch",
"@babel/runtime-corejs2": "7.0.0-beta.56",

This comment has been minimized.

@aduth

aduth Aug 24, 2018

Member

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

@aduth

aduth Aug 24, 2018

Member

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

This comment has been minimized.

@gziolo

gziolo Aug 24, 2018

Member

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

gziolo Aug 24, 2018

Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment