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.

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

Merged
merged 4 commits into from
Feb 12, 2018
Merged

Conversation

aduth
Copy link
Member

@aduth aduth commented Feb 8, 2018

Related: #4661

This pull request seeks to ensure module assignment into the wp global is camel-cased, specifically the edit-post module introduced in #4661 (as wp.editPost, currently wp[ '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.

@aduth aduth added Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Build Tooling Issues or PRs related to build tooling labels Feb 8, 2018
Copy link
Contributor

@youknowriad youknowriad left a 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}
Copy link
Member

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?

Copy link
Member Author

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? 😉

Copy link
Member

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 ❤️

memo[ entryPointName ] = `./${ entryPointName }/index.js`;
entryPointNames.reduce( ( memo, entryPoint ) => {
entryPoint = castArray( entryPoint );
const [ name, path = name ] = entryPoint;
Copy link
Member

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 and path provided they are set
  • if the path is not set, we set its value as name

Copy link
Member Author

@aduth aduth Feb 12, 2018

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.

Copy link
Member

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 {
Copy link
Member

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.

Copy link
Member Author

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.

@gziolo
Copy link
Member

gziolo commented Feb 12, 2018

/home/travis/build/WordPress/gutenberg/blocks/api/raw-handling/slack-markdown-variant-corrector.js
[0] 9:4 error Use @return instead valid-jsdoc
[0]
[0] ✖ 1 problem (1 error, 0 warnings)
[0] 1 error, 0 warnings potentially fixable with the --fix option.

I see an error not related to this PR. Will check master and fix if still there.

Copy link
Member

@gziolo gziolo left a 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")
@aduth
Copy link
Member Author

aduth commented Feb 12, 2018

@gziolo I think I must have rebased while there was an error in master (specifically the one fixed in #4982)

@aduth aduth merged commit 9f160d3 into master Feb 12, 2018
@aduth aduth deleted the update/edit-post-global branch February 12, 2018 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants