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

Tooling: Integrate dependencies from WordPress packages #4705

Merged
merged 3 commits into from
Feb 9, 2018
Merged

Tooling: Integrate dependencies from WordPress packages #4705

merged 3 commits into from
Feb 9, 2018

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Jan 27, 2018 •
edited
Loading

Description

This brings wp-scripts into Gutenberg.

How Has This Been Tested?

Regression tests to confirm nothing has broken.

Run the following code and make sure they still work:

  • npm run build
  • npm run dev
  • npm run gettext-string
  • npm run package-plugin
  • npm run test-unit

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code has proper inline documentation.

@gziolo gziolo added [Status] In Progress Tracking issues with work in progress [Type] Build Tooling Issues or PRs related to build tooling labels Jan 27, 2018
@gziolo gziolo self-assigned this Jan 27, 2018
@ntwb
Copy link
Member

ntwb commented Jan 29, 2018

Tested:

  • npm run build ✅
  • npm run dev ✅
  • npm run test-unit ✅
  • npm run test-unit blocks/test/full-content.js ✅

I also wanted to test the browserslist change.

To test this I ran npm run package-plugin on both this branch and master

I extracted the gutenberg.zip file and performed the following search on the extracted files: ag "\-webkit" --stats

Results for master:

522 matches
13 files contained matches
56 files searched
5693392 bytes searched
0.070173 seconds

Results for this PR update/use-wordpress-packages:

522 matches
13 files contained matches
56 files searched
5693392 bytes searched
0.070173 seconds

I think that makes sense, we didn't expect any changes to vendor prefixes

A 2nd pair of eyes on confirming if any unintended browser compatibility changes haven't gone missing would be great....

@gziolo
Copy link
Member Author

gziolo commented Jan 29, 2018

@ntwb, I want to include @wordpres/jest-preset-default in this PR before we merge. It depends on WordPress/packages#74 where this package is about to be extracted :)

@ntwb ntwb requested a review from aduth January 29, 2018 23:24
@gziolo gziolo force-pushed the update/use-wordpress-packages branch 3 times, most recently from 3ee0168 to 72acb99 Compare February 7, 2018 13:54
@gziolo gziolo added [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. and removed [Status] In Progress Tracking issues with work in progress labels Feb 7, 2018
@gziolo
Copy link
Member Author

gziolo commented Feb 7, 2018

@ntwb - this is ready for review. I included Jest preset and had to fully regenerate package-lock.json to make it work. I also moved Babel config to package.json, but I can also move it back if we don't like this change.

@gziolo
Copy link
Member Author

gziolo commented Feb 7, 2018

Sharing this link with Next.js tutorial how to extend their default configs: https://zeit.co/blog/next5#universal-webpack-and-next-plugins. Another way how we could tackle presets in the follow-up PRs.

ntwb
ntwb approved these changes Feb 8, 2018
View reviewed changes
Copy link
Member

@ntwb ntwb left a comment

Choose a reason for hiding this comment

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

Fantastic to see this all coming together now, LGTM 👍

youknowriad
youknowriad reviewed Feb 8, 2018
View reviewed changes
package.json
@@ -151,12 +142,12 @@
"fixtures:generate": "npm run fixtures:server-registered && cross-env GENERATE_MISSING_FIXTURES=y npm run test-unit",
"fixtures:regenerate": "npm run fixtures:clean && npm run fixtures:generate",
"package-plugin": "./bin/build-plugin-zip.sh",
"test-unit": "jest",
"test-unit": "wp-scripts test",
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is more a package issue, but I wonder if this command should be wp-scripts unit-test to allow for other kind of test commands.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the future, we might have also e2e support but we are very far away from this :) We can rename it or add a flag like --type=unit or something along those lines. At the moment it's a simple wrapper for jest.

youknowriad
youknowriad reviewed Feb 8, 2018
View reviewed changes
test/unit/pegjs-transform.js Outdated
@@ -1,16 +0,0 @@
const pegjs = require( 'pegjs' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we need this file anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, some tests depend on the parser inside Gutenberg. I moved this to jest-preset-default in case someone ever uses the code that loads those files. Technically it all stays the same.

Copy link
Contributor

@youknowriad youknowriad Feb 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Minor: Maybe, I'd have preferred this one to stay Gutenberg specific as it's not really a common thing to use pegjs

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'm fine with leaving it here as suggested. I will update PR.

@gziolo gziolo force-pushed the update/use-wordpress-packages branch from 72acb99 to 384e590 Compare February 9, 2018 10:14
gziolo
gziolo commented Feb 9, 2018
View reviewed changes
package.json
"testMatch": [
"<rootDir>/(blocks|components|date|editor|element|i18n|data|utils|edit-post)/**/test/*.js"
],
"timers": "fake",
"transform": {
"^.+\\.jsx?$": "babel-jest",
Copy link
Member Author

@gziolo gziolo Feb 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@youknowriad it's back here. I had to reference babel-jest here because it doesn't merge options with preset. I opened an issue in Jest to fix it: jestjs/jest#5505.

@gziolo gziolo merged commit 6123762 into master Feb 9, 2018
@gziolo gziolo deleted the update/use-wordpress-packages branch February 9, 2018 15:02
This was referenced Feb 11, 2018
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youknowriad youknowriad youknowriad left review comments

@ntwb ntwb ntwb approved these changes

@aduth aduth Awaiting requested review from aduth

Assignees

@gziolo gziolo

Labels
[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gziolo @ntwb @youknowriad