-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe 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. |
||
"babel-eslint": "^8.2.2", | ||
"babel-plugin-react-native-classname-to-style": "^1.2.1", | ||
"babel-plugin-react-native-platform-specific-extensions": "^1.0.1", | ||
|
@@ -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 commentThe 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 commentThe 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. |
||
"@wordpress/deprecated": "^1.0.0-alpha.2", | ||
"@wordpress/hooks": "^1.2.1", | ||
"@wordpress/i18n": "^1.1.0", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
|
||
import { createElement } from '@wordpress/element'; | ||
import jsdom from 'jsdom-jscore'; | ||
import jsdomLevel1Core from 'jsdom-jscore/lib/jsdom/level1/core'; | ||
|
||
global.wp = { | ||
element: { | ||
|
@@ -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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. excellent, thanks for this clarification! |
||
global.window.Node = jsdomLevel1Core.dom.level1.core.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.
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.
The package is actually called like that (see its package definition here) so, it makes sense to keep its original name.
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 thegutenberg/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 itsjest.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.
👍 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.
Thanks for pointing this out as well, brings useful insight