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

Packages: Add common utilities to WP.com block editor package #32294

Merged
merged 5 commits into from Apr 17, 2019

Conversation

mmtr
Copy link
Member

@mmtr mmtr commented Apr 16, 2019

Changes proposed in this Pull Request

Add a new folder for all the common utilities we need to run in both block editor environments (Gutenframe and WP Admin).

So far, these utilities are the rich text extensions to add the missing formats in Core (justify, underline) and the "Switch to Classic" button on the "More tools" menu.

The logic of these utilities were previously maintained on the gutenberg-wpcom.js file, so the files added to the common/ folder on this PR contain the same code but using ES2015+ syntax.

The documentation has also changed in order to make more obvious there are 2 environments we need to support. Note that I also grouped the previous scripts in a calypso/ folder to reflect that the utilities inside this folder will be run only in a iframed block editor context.

Given that this is adding a new entry file to the build script, I decided to split the bundle script into 3 smaller build scripts (one for each entry file), so we don't have a very long script command. The build scripts runs now all the smaller build scripts in parallel (using npm-run-all) and requires an explicit output path (reason why the clean script is not needed anymore).

Testing instructions

Check D26970-code

@mmtr mmtr added [Status] In Progress [Goal] Gutenberg Working towards full integration with Gutenberg Packages labels Apr 16, 2019
@mmtr mmtr requested a review from a team as a code owner April 16, 2019 06:06
@mmtr mmtr self-assigned this Apr 16, 2019
@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@mmtr mmtr added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Apr 16, 2019
Server-side handlers of the different communication channels we establish with the client-side when Calypso loads the iframed block editor. See [`calypsoify-iframe.jsx`](https://github.com/Automattic/wp-calypso/blob/master/client/gutenberg/editor/calypsoify-iframe.jsx).
- `/common`: Logic than runs on both environments (WP Admin and Calypso).
- `/calypso`: Logic than runs only on the Calypso iframed block editor.
- `/wp-admin`: Logic than runs only on the WP Admin block editor.
Copy link
Contributor

@kwight kwight Apr 16, 2019

Choose a reason for hiding this comment

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

Might as well remove this; it's not present, and not anticipated in the near future.

@kwight
Copy link
Contributor

kwight commented Apr 16, 2019

Awesome work ❤️

I did notice the minified scripts are missing from a manual build, and comparing the artifacts build versus the manual build shows a lot of differences (eval seems like not a great thing?) – should they be aligned, and which would be preferable?

Screen Shot 2019-04-16 at 9 36 54 AM

The documentation has also changed in order to make more obvious there are 2 environments we need to support. Note that I also grouped the previous scripts in a calypso/ folder to reflect that the utilities inside this folder will be run only in a iframed block editor context.

This is magnificent, thank you! 🎉

Copy link
Contributor

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

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

Looks like some good improvements here. I don't see any reason to block merging this.

Thanks @mmtr for updating this code. Those scripts are starting to make me long for our Makefile again… 😉

"build:common": "npm run bundle -- common='./src/common'",
"build:calypso-iframe-bridge-server": "npm run bundle -- calypso-iframe-bridge-server='./src/calypso/iframe-bridge-server.js'",
"build:calypso-tinymce": "npm run bundle -- calypso-tinymce='./src/calypso/tinymce.js'",
"build": "npm-run-all --parallel \"build:* -- {@}\" --"
Copy link
Member

Choose a reason for hiding this comment

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

If this gets too funky at some point, you could also create a simple Webpack config for this package that inherits and extends the one from Calypso-build, similarly to client/notifications/.

@obenland obenland added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 16, 2019
@mmtr
Copy link
Member Author

mmtr commented Apr 17, 2019

I did notice the minified scripts are missing from a manual build, and comparing the artifacts build versus the manual build shows a lot of differences (eval seems like not a great thing?) – should they be aligned, and which would be preferable?

Yeah, this is normal since the commands used in the manual build are different from the automatic CircleCi build.

The CircleCI job executes two commands:

  1. Build for the unminified scripts.
SOURCEMAP='inline-cheap-source-map' npx lerna run build --scope='@automattic/wpcom-block-editor' -- -- --output-path=$CIRCLE_ARTIFACTS/wpcom-block-editor

SOURCEMAP='inline-cheap-source-map' is used for generating a more readable build file in order to ensure compliance with the plugin guidelines.

  1. Build for the minified scripts.
NODE_ENV=production npx lerna run build --scope='@automattic/wpcom-block-editor' -- -- --output-path=$CIRCLE_ARTIFACTS/wpcom-block-editor --output-filename=[name].min.js

NODE_ENV=production is used for enabling the minification.


I was thinking in wrapping these 2 commands in a single npm script that replaces the current build script, so both builds (manual and automatic) will generate the same dist. But I didn't see any other script in Calypso that defines a NODE_ENV env var, which is the main reason why I didn't go for it, since this would prevent to skip the minification if we really don't want it.

Probably in this case we want consistency rather than flexibility, so I'm going to commit a change grouping the 2 commands so npx lerna run build --scope='@automattic/wpcom-block-editor' will generate both the unminified and minified bundles.

Edit: changed in a12f1c8

@sirreal @simison @ockham let me know if should take another approach here.

@mmtr mmtr merged commit fccc289 into master Apr 17, 2019
@mmtr mmtr deleted the update/wpcom-block-editor-common-utilities branch April 17, 2019 05:13
mmtr added a commit that referenced this pull request Apr 17, 2019
…on (#32344)

Remove an unintended semicolon on the "Switch to Classic" button introduced by #32294.
"@wordpress/url": "2.3.3",
"jquery": "^1.12",
"lodash": "4.17.11",
"react": "16.8.6",
"tinymce": "4.8.5"
},
"devDependencies": {
Copy link
Member

Choose a reason for hiding this comment

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

A peculiarity of monorepo: devDependencies in non-root packages don't really mean anything. It's preferable to never include them, any devDependencies must be in the root package.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wouldn't that cause a conflict with the import/no-extraneous-dependencies ESLint rule?

Screen Shot 2019-04-18 at 11 07 14

Copy link
Member

Choose a reason for hiding this comment

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

Might be. :-) It's not ideal — all I know it's causing trouble. See e.g.: #32383

cc @blowery

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Goal] Gutenberg Working towards full integration with Gutenberg Packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants