-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Build: Assign edit-post
global as wp.editPost
#4966
Conversation
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.
Not enough skills to review the webpack plugin properly but this seems to work great, I'm always impressed with the way you always find a solution to all issues :)
* | ||
* @param {Object} compiler Webpack compiler | ||
* | ||
* @return {void} |
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.
Do we really need to explicitly document that function doesn't return anything? Wouldn't it enough to assume that a function by default doesn't return value?
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.
Do we really need to explicitly document that function doesn't return anything? Wouldn't it enough to assume that a function by default doesn't return value?
There's some precedent of this in existing core documentation. In general I'd be inclined to agree with you that it could be omitted for undefined
return, but I could also see an argument for consistency / explicitness. Maybe worth bringing up in tomorrow's JS meeting? 😉
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.
Docs again? Count me in ❤️
webpack.config.js
Outdated
memo[ entryPointName ] = `./${ entryPointName }/index.js`; | ||
entryPointNames.reduce( ( memo, entryPoint ) => { | ||
entryPoint = castArray( entryPoint ); | ||
const [ name, path = name ] = entryPoint; |
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.
Just wanted to double check if I understand how destructuring works here:
- if there is a
name
andpath
provided they are set - if the
path
is not set, we set its value asname
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.
Just wanted to double check if I understand how destructuring works here:
This is correct. I've added a clarifying inline comment in d3a43bf.
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, you don't always see defaults set here, using another param. Quite nice that is is possible 👍
* | ||
* @see webpack.TemplatedPathPlugin | ||
*/ | ||
class CustomTemplatedPathPlugin { |
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.
Do we plan to extract it and move to WordPress/packages
at some point? It might be useful for other repositories.
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.
Do we plan to extract it and move to
WordPress/packages
at some point? It might be useful for other repositories.
Yeah, seems like a generally useful plugin.
I see an error not related to this PR. Will check |
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 checked job logs on Travis. Everything looks as advertised. I can see that Webpack produces editPost
bundle and Cypress' tests pass.
I don't have enough Webpack experience to fully validate plugin's code. However I'm satisfied with the result of this PR 👍
directory of resolved request is not sufficient because the resolved file of WordPress dependencies is within the `build-module` directory and would output as such. Instead format in such a way that we can call basename on the _raw_ requested path (e.g. `./editor` -> "editor", `./node_modules/@wordpress/hooks" -> "hooks")
938a7c1
to
81a366c
Compare
Related: #4661
This pull request seeks to ensure module assignment into the
wp
global is camel-cased, specifically theedit-post
module introduced in #4661 (aswp.editPost
, currentlywp[ 'edit-post' ]
). As our build process is generalized, this required some customization of our Webpack build configuration, including the introduction of a custom Webpack plugin for enhancing tags to include custom keys (e.g. add[dir]
in addition to[name]
,[id]
, etc). This plugin could be easily externalized to a separate module, likely under the WordPress namespace.Testing instructions:
Verify that the build outputs file as expected and that the editor loads without errors.