-
Notifications
You must be signed in to change notification settings - Fork 56
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
Update to latest GB master #80
Conversation
* using the @wordpress/blocks packages now, and directly from src * For some reason, Flow doesn't like pointing to the root of the blocks package (while it's not complaining for the same reason for the `element` package). Having the flow config pointing inside the `src` dir of the package now. * Need to expose `window.Node` for a couple of its constants. Using jsdom's definitions for that. * Code block needs a css class defined (wp-block-code) in the innerHTML for validation to succeed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sharing this @hypest! - questions / comments below. I'd approve then 👍
@@ -85,6 +85,7 @@ munge_underscores=true | |||
|
|||
module.name_mapper='^[./a-zA-Z0-9$_-]+\.\(bmp\|gif\|jpg\|jpeg\|png\|psd\|svg\|webp\|m4v\|mov\|mp4\|mpeg\|mpg\|webm\|aac\|aiff\|caf\|m4a\|mp3\|wav\|html\|pdf\)$' -> 'RelativeImageStub' | |||
module.name_mapper='@gutenberg' -> '<PROJECT_ROOT>/gutenberg' | |||
module.name_mapper='@wordpress/blocks' -> '<PROJECT_ROOT>/gutenberg/packages/blocks/src' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually not sure we should name this @wordpress
as blocks are actually @gutenberg
's. As a result of this observation we may also consider how other mappers are called, which I'm not sure of, but for Gutenberg blocks it sounds like it's definitely @gutenberg
, wdyt?
Also, may I ask why do we set this here in Flow, and is not set in the module mapper plugin in .babelrc
to indicate an alias? That's how I'd approached it, and would like to learn if that's a feasible path or not, and why this would be preferred, if the question first of all makes sense (i.e. can Flow be used for the same thing?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually not sure we should name this @WordPress as blocks are actually @gutenberg's.
The package is actually called like that (see its package definition here) so, it makes sense to keep its original name.
why do we set this here in Flow, and is not set in the module mapper plugin in .babelrc to indicate an alias
It's not an alias as far as the build pipeline is concerned. For Metro, @wordpress/blocks
is a normal package and tries its best to resolve it. Metro being the smart tool it is, it actually discovers the package automagically inside the gutenberg/packages
folder.
We gradually have moved away from having to define aliases in Babel as the Gutenberg code has been npm-package-ized more and more. Previously, we had to "mock" those packages to mimic what Webpack is doing on the Gutenberg build side. But nowadays, the published Gutenberg packages can be easily picked up by Metro without alias tricks. Some packages (namely, the editor
package` is still not published to NPM and that's why it's practically the only alias we have left in Babel.
But, Flow
is not as smart as Metro. We need to give more direct instructions to Flow to go find the packages.
While on the subject, Jest
(our testing tool) is also not as smart as Metro and that's why we have similar aliases in its jest.config.js
file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The package is actually called like that (see its package definition here) so, it makes sense to keep its original name.
👍 thanks for pointing to the original definition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the detailed explanation about moving away from module resolver. I wonder why that wouldn't work then off the shelve for me (automagically as you say) when working on a similar PR in https://github.com/wordpress-mobile/gutenberg-mobile/tree/update/use-latest-gb-master
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While on the subject, Jest (our testing tool) is also not as smart as Metro and that's why we have similar aliases in its jest.config.js file
Thanks for pointing this out as well, brings useful insight
@@ -8,6 +8,7 @@ | |||
}, | |||
"devDependencies": { | |||
"@wordpress/babel-preset-default": "^1.1.2", | |||
"@wordpress/block-serialization-spec-parser": "^1.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't realize this was needed. May I ask, even if it's a trivial question, how you come to know this needs to be set as a dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that crops up when you try to run the tests actually. If you try without the dep you'll notice Jest complaining. Haven't investigated why it happens only on the testing pipeline though.
@@ -61,6 +62,7 @@ | |||
}, | |||
"dependencies": { | |||
"@wordpress/autop": "^1.0.6", | |||
"@wordpress/compose": "^1.0.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this included as a dependency here, but then is denylisted here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so, Metro being the smart tool it is, it actually tries to resolve the packages by looking in the local filesystem. So, when you do want to use the remote package from NPM you need to denylist the local package dir.
@@ -18,3 +19,7 @@ doc.implementation.createHTMLDocument = function( html ) { | |||
|
|||
// `hpq` depends on `document` be available globally | |||
global.document = doc; | |||
|
|||
if ( ! global.window.Node ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! question, does defining this in this way fix the red screen I got saying undefined is not an object (evaluating '_window$Node.TEXT_NODE')
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! So, turns out that recent Gutenberg changes exposed the need for those constants from window.Node
that were previously handled from dom-react
and our jsdom-core
dep. But yeah, with the changes we apparently need to manually add the support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excellent, thanks for this clarification!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, thanks for thoroughly answering (as usual) my questions @hypest . I'll now test the PR and approve if all good 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested, LGTM
…pport arbirtary focus codes from RN (#81)
Ability to build a binary via jitpack
Update to the currently latest Gutenberg master branch.
Changes:
package (while it's not complaining for the same reason for the
element
package). Having the flow config pointing inside thesrc
dirof the package now.
window.Node
for a couple of its constants. Usingjsdom's definitions for that.
for validation to succeed