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.

Sign up for GitHub

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

Jump to bottom

Gutenberg: include @wordpress/data package #26203

Merged
merged 1 commit into from
Jul 24, 2018
Merged

Gutenberg: include @wordpress/data package #26203

merged 1 commit into from
Jul 24, 2018

Conversation

vindl
Copy link
Member

@vindl vindl commented Jul 19, 2018 •
edited

Part of #26180

Set up a Calypso test branch for @wordpress/data package and add some basic tests for it.

Testing instructions

  • npm run test-client client/gutenberg/editor/test/index.js
  • Calypso smoke test

@vindl vindl added [Status] In Progress [Goal] Gutenberg Working towards full integration with Gutenberg labels Jul 19, 2018
@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=try/wordpress-data

@vindl vindl self-assigned this Jul 19, 2018
@vindl vindl changed the title WIP: Gutenberg: experimental include of @wordpress/data package [WIP] Gutenberg: experimental include of @wordpress/data package Jul 19, 2018
@vindl vindl requested a review from a team July 19, 2018 21:32
blowery
blowery reviewed Jul 19, 2018
View reviewed changes
package.json Outdated
@@ -27,6 +27,8 @@
"@babel/preset-react": "7.0.0-beta.44",
"@babel/preset-stage-2": "7.0.0-beta.44",
"@babel/runtime": "7.0.0-beta.44",
"@wordpress/data": "1.0.0",
"ajv": "6.5.2",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the ajv dep?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was running into this during installation:

npm WARN ajv-keywords@3.2.0 requires a peer of ajv@^6.0.0 but none is installed. You must install peer dependencies yourself.

Haven't checked if it's really necessary yet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah ok. i think that's a npm issue. we see it on master too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

webpack/schema-utils#23 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the additional context! As expected, removing it didn't cause any issues on this branch.

@vindl vindl mentioned this pull request Jul 19, 2018
Closed
27 tasks
@gwwar
Copy link
Contributor

gwwar commented Jul 20, 2018

💖looks promising. Let's move over testing registerStore and dispatch into a unit test. Since Gutenberg is developing so quickly this will help let us know if anything has changed with future package updates. We can maybe remove this later once things stabilize a bit.

After we verify that we haven't added bloat to other bundles (besides the gutenberg section) http://iscalypsofastyet.com/branch?branch=try/wordpress-data I think we can land this.

@gwwar
Copy link
Contributor

gwwar commented Jul 20, 2018

CI errors are from having an empty test file:

 FAIL  client/gutenberg/editor/test/index.js
  ● Test suite failed to run

    Your test suite must contain at least one test.

      at node_modules/jest/node_modules/jest-cli/build/test_scheduler.js:256:22

Summary of all failing tests
 FAIL  client/gutenberg/editor/test/index.js
  ● Test suite failed to run

    Your test suite must contain at least one test.

      at node_modules/jest/node_modules/jest-cli/build/test_scheduler.js:256:22

@vindl vindl changed the title [WIP] Gutenberg: experimental include of @wordpress/data package Gutenberg: include @wordpress/data package and add tests for it Jul 23, 2018
@vindl vindl changed the title Gutenberg: include @wordpress/data package and add tests for it Gutenberg: include @wordpress/data package Jul 23, 2018
@vindl vindl added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Tests] Includes Tests and removed [Status] In Progress labels Jul 23, 2018
@vindl
Copy link
Member Author

vindl commented Jul 23, 2018 •
edited

Let's move over testing registerStore and dispatch into a unit test.

@gwwar moved these and a couple of other @wordpress/data exported functions to a unit test. This should also resolve the CI errors.

After we verify that we haven't added bloat to other bundles (besides the gutenberg section) http://iscalypsofastyet.com/branch?branch=try/wordpress-data I think we can land this.

Delta: no changes in production JS.

gwwar
gwwar reviewed Jul 24, 2018
View reviewed changes
client/gutenberg/editor/test/index.js
@@ -0,0 +1,103 @@
/**
* @format
* @jest-environment jsdom
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this do? Do we have some new ✨ automation options?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gwwar Without it the test will produce this error:

ReferenceError: window is not defined
at registerReducer (node_modules/@wordpress/data/build/registry.js:215:5)
...

Quoting the testing docs:

It's very possible that your tests will assume the existence of a browser environment to work properly. The test runner we use, Jest, uses the browser-like environment jsdom. We default to a node-like environment to make tests faster. If some tests require another environment, you can add /** @jest-environment jsdom */ docblock. Check this Jest doc to learn more.

(Reference: https://github.com/Automattic/wp-calypso/blob/a9150ed7ded1f97235bd4e069639ea9b1ab3d6e7/docs/testing/component-tests.md#set-up-a-test-environment).

It looks like the registerReducer depends on window, so I had to include this to make tests happy.

gwwar
gwwar reviewed Jul 24, 2018
View reviewed changes
client/gutenberg/editor/test/index.js
const reducer = () => 'tea';

const store = registerReducer( 'reducer', reducer );
expect( store.getState() ).toEqual( 'tea' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍵

gwwar
gwwar approved these changes Jul 24, 2018
View reviewed changes
Copy link
Contributor

@gwwar gwwar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @vindl 🎉 this is good to ship after we resolve package conflicts + make sure the shrinkwrap matches.

@vindl vindl removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 24, 2018
@vindl vindl merged commit 6647210 into master Jul 24, 2018
@vindl vindl deleted the try/wordpress-data branch July 24, 2018 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blowery blowery blowery left review comments

@gwwar gwwar gwwar approved these changes

Assignees

@vindl vindl

Labels
[Goal] Gutenberg Working towards full integration with Gutenberg [Tests] Includes Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@vindl @matticbot @gwwar @blowery