Plugin: Implement block registering API #289

youknowriad · 2017-03-17T13:07:08Z

Questions

I was thinking about adding unit tests for these fonctions, but setting up QUnit is not an easy task with our setup. Do you think we could use mocha maybe?
Also: Using a library like lodash here seems appropriate, what are your thoughts on adding it?
We're using a tuple (namespace, name) to identify a block, should we instead merge them into one id ( namespace/name )?

youknowriad · 2017-03-17T13:15:02Z

external/@wordpress/blocks/index.js

 }

 /**
 * Returns all registered blocks.
 *
- * @return {Object} Block settings keyed by block name
+ * @return {Array} Blocks settings


It's not keyed by block name because the block name is not unique, it's unique inside a namespace only.

Yeah, I'm worried we'll find the namespacing will get in our way more than it helps. I'm not totally convinced about it. It's nice to have the consistency with similar APIs for the REST API, but I wonder if we could just allow a simple name and perhaps encourage a convention around including some namespace in the name itself: like "wp/text" (or without namespace for "core" types).

What are the expected uses of this function? Do you think we will need the ability to pull a specific block out of the list? If so, it might be a good idea to return an array keyed by namespace then block name, as the blocks are stored.

Also, Block settings is better wording.

What are the expected uses of this function?

This is a good point. Seems nice as an external API, but I don't see an obvious use-case for us initially.

From my experience with the multi-instance prototype. This function was used in two places:

Fetching all the blocks to show in the block inserter (Array is better here, because we need to group them by category and not by namespace)

Fetching all the blocks to find the blocks that a block can be switched to (The filtering happens using one of the settings key and not the namespace).

So IMO it's better to return a simple array.

Regarding the name, I'm leaning towards removing the namespace and encouraging a convention

Regarding the name, I'm leaning towards removing the namespace and encouraging a convention

Encouraging a convention is not enough, it needs to be enforced otherwise people won't follow it.

Though this can be done either way -

if ( ! /^\w+/\w+$/\.test( blockName ) ) { throw new Error( 'Use a namespace please' ); }

Repeating my comment elsewhere, I think we can let go of namespace. The name should be unique and descriptive anyways because the user would interact and search for it.

aduth · 2017-03-17T14:56:27Z

external/@wordpress/blocks/index.js


+	blocks[ namespace ][ block ] = Object.assign(
+		{ name: block, namespace },


If we call it name on the object, should we just accept the argument labeled as name as well?

aduth · 2017-03-17T14:56:45Z

external/@wordpress/blocks/index.js

@@ -1,5 +1,7 @@
 export { default as Editable } from './components/editable';

+const blocks = {};


Might be nice to provide a DocBlock for this.

nylen · 2017-03-17T15:07:29Z

I was thinking about adding unit tests for these functions [...] Do you think we could use mocha maybe?

Yes, let's go ahead and do this please :) Whatever we choose should support browser testing as well.

I agree I like Mocha better, but core uses QUnit. Do you see this causing problems in the future if we need to integrate both? What are the difficulties in adding QUnit?

Also: Using a library like lodash here seems appropriate, what are your thoughts on adding it?

+1. WP core already bundles Underscore, though Lodash is nicer to work with and we would be able to include selective imports in a build process.

We're using a tuple (namespace, name) to identify a block, should we instead merge them into one id ( namespace/name )?

If we do keep the namespace and name separate, this makes it easier to test and enforce guidelines for block authors using these fields correctly.

youknowriad · 2017-03-17T15:12:32Z

If we do keep the namespace and name separate

The problem with keeping the name and namespace separate is that each time we'll need to retrieve the block settings we'll do getBlockSettings( blockState.name, blockState.namespace) which means we need to store a namespace and a name in the block state and in the generated markup (or metadata).  while we could keep it simpler 

nylen · 2017-03-17T15:15:26Z

getBlockSettings( blockState.name, blockState.namespace )

Good point, that is not ideal :)

As long as we can do something like #289 (comment), then combining the namespace and name makes sense to me.

youknowriad · 2017-03-17T15:31:02Z

I updated the API and dropped the namespace.

Do you see this causing problems in the future if we need to integrate both? What are the difficulties in adding QUnit?

My experience with QUnit is equal to Zero :). So after reading the docs, I figured out that integrating it with our build system means having a separate webpack.config for the tests and import all the test files on a test entry point file. Not so handy IMO, but maybe I'm missing a better way to setup qunit. If someone could help on setting up QUnit (could be a separate PR), it would be awesome!!!

nylen · 2017-03-17T15:50:56Z

I would very much prefer that we start now with the rule of "do not merge code unless it has tests".

Also, mocha is going to be much nicer to work with than QUnit, and thinking about it a bit more, any difficulties with integration are likely to be pretty minor. I'll try to add it here.

nylen · 2017-03-17T18:38:41Z

@mtias I'm replying to your #289 (comment) here because the thread has disappeared after a rebase.

Repeating my comment elsewhere, I think we can let go of namespace. The name should be unique and descriptive anyways because the user would interact and search for it.

I am not sure whether you mean letting go of namespace as a separate argument, or the concept entirely. I agree that using the namespace as a separate argument is going to lead to more complicated code, removing it in 65d60a6 has already simplified things a good bit.

But we really need to require that blocks use a plugin-specific prefix for their internal ID/name/slug/whatever you call it, and in general we need to start thinking about how blocks can be named and how their resulting  tags will look. Otherwise there will be collisions and it will be unpleasant.

There is a lot of precedent in WP core for doing similar things. The reasoning is laid out really well in this blog post: https://nacin.com/2010/05/11/in-wordpress-prefix-everything/

Here are a couple more specific examples:

IMO, it's also worth considering whether we need to version blocks similarly to REST API endpoints. This is something we could add later on if we need it, though.

mtias · 2017-03-17T19:00:29Z

@nylen yes, sorry for the confusion, I mean using specific argument or / syntax for namespacing and just doing myplugin-map or myplugin_map.

nylen · 2017-03-17T19:07:47Z

I'm not planning to spend a lot of time on it, beyond making it work and writing some simple tests, but enforcing a very specific syntax is a good thing. We can always relax the rules later when more needs become apparent, but we can't do the reverse when people misuse the functionality provided.

aduth · 2017-03-17T19:13:50Z

external/@wordpress/blocks/index.js

 */
 export function getBlocks() {
-
+	return Object.values( blocks );


This will probably require pulling in babel-plugin-transform-runtime, otherwise will error on browsers where this is not yet implemented.

aduth · 2017-03-17T21:48:28Z

.babelrc

@@ -1,9 +1,8 @@
 {
 	"presets": [
-		[ "latest", {
-			"es2015": {
-				"modules": false


This was important to have in order to achieve Webpack tree shaking.

Hm, interesting. At one point I was getting an error until I removed this (http://stackoverflow.com/a/42834190) but now it is working fine.

youknowriad · 2017-03-17T21:54:12Z

.babelrc

-				"modules": false
-			}
-		} ]
+		[ "latest", "es2015" ]


Also, es2015 is included in latest, I think it's not needed here

Is there a further change we should make here? I'm not sure if you can do latest: { modules: false } for example, or how to test whether this is working as intended.

No, the last change is good 👍

Yes, hard to know if it's working correctly. I guess we'll see when we import some function for lodash. We could check that it's not including the whole library

nylen · 2017-03-17T21:54:38Z

This now has mocha unit tests 🎉

A few notes about the test setup:

I ignored yarn.lock, but maybe we should consider committing this file instead?
I'm using mocha-webpack to run the tests through the same webpack transformations as the actual build. It's pretty important to keep our build and our test environment in sync like this - it's something that's currently causing us problems in Calypso (Testing: Test build environment is vastly different than dev/prod. Automattic/wp-calypso#12203). See this article for details, configuration explanations, and next steps for getting testing working in a browser: http://randycoulman.com/blog/2016/03/22/testing-with-mocha-and-webpack/
I added dirty-chai because I don't really trust property-based assertions. It's easy to make a typo like expect( x ).to.be.ture; - this will appear to succeed but will not actually do anything.

youknowriad · 2017-03-17T22:01:59Z

test/blocks/index.js

+import chai, { expect } from 'chai';
+import dirtyChai from 'dirty-chai';
+
+import * as blocks from '@wordpress/blocks';


Is there a reason why you would use * instead of explicit imports?

I have no preference either way.

In this particular case, it's not making any difference, but worth noting that in other use cases when importing from external libraries, it can have an impact on tree shaking

import * as _ from 'lodash'; console.log( _.uniqueId() ); // This will include the whole lodash import { uniqueId } from 'lodash'; console.log( uniqueId() ); // This will include only uniqueId

youknowriad · 2017-03-17T22:03:14Z

test/blocks/index.js

@@ -0,0 +1,73 @@
+import mocha from 'mocha';


Should we use the /** External/Internal dependencies ** comments (similar to Calypso)?

Yep, probably :)

nylen · 2017-03-17T22:07:21Z

In e157c9a I changed name to slug. So far, we're really dealing with the internal ID of the block, which I am calling a slug because that's the WordPress term for a dashed identifier, including a plugin's ID.

I'm open to suggestions here, but I don't think it should be called name. The name is the human-readable block name, which doesn't exist yet but will be different and probably not necessarily unique.

youknowriad · 2017-03-17T22:10:23Z

I ignored yarn.lock, but maybe we should consider committing this file instead?

On the initial build PR. I've used yarn as well but if we commit the yarn.lock, all contributors should use yarn to avoid making yarn.lock out of sync when adding dependencies. @aduth raised a good point, that this would add a new thing to learn for contributors, and we may want to stick with npm for now

youknowriad · 2017-03-17T22:18:58Z

This is looking solid for me. Thanks, @nylen for the test infra. Waiting for a 👍 to merge. We could iterate later if we need to change the block slug format or anything else.

aduth

Did you look to see if there's a chance we could integrate QUnit without too much trouble? Seems like it will suffer many of the same gotchas as Mocha, but if we could pull it off would certainly make eventual integration a fair bit less... complicated.

aduth · 2017-03-18T00:00:22Z

test/blocks/index.js

+
+import * as blocks from '@wordpress/blocks';
+
+chai.use( dirtyChai );


If we're going to pull in and assume plugins to be used consistently, could we somehow apply it as part of the overall test initialization?

I extracted this to a test/boot.js

aduth · 2017-03-18T00:02:00Z

test/blocks/index.js

+chai.use( dirtyChai );
+
+describe( 'blocks API', () => {
+	// TODO: We probably want a way to undo this, and split this logic out into


Would blocks.unregisterBlock be a reasonable reasonable to work around this problem?

aduth · 2017-03-18T00:08:09Z

webpack.config.test.js

+
+const config = module.exports = Object.assign( {}, baseConfig, {
+	target: 'node',
+	externals: [ nodeExternals() ],


I see it in mocha-webpack's docs, and even upon reading the docs of webpack-node-externals itself, it's still not clear why this is necessary. Does it make the build more performant? Is it any different than:

resolve: { modules: baseConfig.resolve.modules.filter( ( source ) => source !== 'node_modules' ) }

I tried without it and using your alternative. I don't understand all what's happening here but it's not working without this.

aduth · 2017-03-18T00:12:20Z

package.json

@@ -11,23 +11,30 @@
    "editor"
  ],
  "scripts": {
+    "test": "mocha-webpack test/",


Seeing that mocha-webpack has a --webpack-config option and our Webpack config already includes some environment-specific handling, do you have any opinion on whether it might make sense to merge webpack.config.test.js into the base config and change this to:

"test": "NODE_ENV=test mocha-webpack --webpack-config webpack.config.js test/"

Trying to think of ways to avoid root directory becoming too sprawling with files.

aduth · 2017-03-21T13:36:00Z

Issue with mocha-webpack is that it won't respect our custom entry definitions:

Maybe you noticed that entry, output.filename and output.path are completely missing in this config. mocha-webpack does this automatically for you and if you would specify it anyway, it will be overridden by mocha-webpack.

https://www.npmjs.com/package/mocha-webpack

- No need for a separate `validateBlockSlug` function - More thorough and more isolated tests

nylen · 2017-03-21T21:00:12Z

Thanks, @youknowriad and @aduth for working on this and for reviewing.

In a512271 I added blocks.unregisterBlock, which allowed for some improvements to the tests and for getting rid of the separate function for validateBlockSlug.

I'm not having any problems with npm test or npm run build, and I don't follow the globals issues mentioned in #289 (comment). Is there anything left to do here?

One remaining question here is whether to use QUnit or Mocha for test infrastructure. I find QUnit very awkward to work with.

In terms of eventual integration with WP core, in the worst case this is going to mean that we add a separate Travis CI job to run Mocha tests. I think this is OK, because there will be lots of tests for this project, so they will take a while to run 😉

I think the added developer convenience of using Mocha is well worth it, and maybe we can also start a new standard for core JS tests.

So, in short, IMO this is ready to merge so we can move forward.

This was from `mocha-webpack`, which is no longer used, so this entry is no longer needed.

Comments addressed

youknowriad · 2017-03-22T10:00:46Z

I don't think it's related to this PR, but we're having errors like TypeError: n(...) is not a function on the plugin page. Seems like a conflict between the webpack build and some WordPress global variable? Any idea about this?

nylen · 2017-03-22T12:30:37Z

I think those errors are related to this PR. Here are the ones I'm seeing (and none in master after a rebuild):

Seems like we need to add some integration tests for the plugin page as well.

youknowriad · 2017-03-22T12:54:22Z

I found that babel transform-runtime plugin is causing these errors, but I can't find why exactly.

youknowriad · 2017-03-22T13:12:07Z

Replacing transform-runtime with babel-polyfill fixes the bug, but we can not include the polyfill in the three modules (duplicate), I'm thinking of adding the polyfill as an external script tag. Thoughts?

aduth · 2017-03-22T13:49:37Z

65066e1 fixes errors from Webpack.

There's another error from registerBlock for how we're using old signature for the text block. Throwing errors crashes the entire editor initialization. Maybe a little heavy handed? Perhaps we can consider console.error instead.

youknowriad · 2017-03-22T13:59:49Z

Thanks for the fix @aduth I fixed the npm test command to correctly check lint errors and fix the remaining lint issues in b95c775

youknowriad · 2017-03-22T14:07:26Z

I fixed the last registerBlock error in e5f72ca and the React minification warning. I'm rebasing (too many junk commits) and merging. Tks @aduth and @nylen. We can make changes to these on follow-up PRs if needed

aduth · 2017-03-22T17:38:25Z

I'd still like some feedback on my earlier point:

Throwing errors crashes the entire editor initialization. Maybe a little heavy handed? Perhaps we can consider console.error instead.

youknowriad · 2017-03-22T17:51:11Z

@aduth I agree, We shouldn't crash on wrong blocks, but just ignore them.

👍 for console.error for me

youknowriad added the Framework Issues related to broader framework topics, especially as it relates to javascript label Mar 17, 2017

youknowriad self-assigned this Mar 17, 2017

youknowriad requested a review from aduth March 17, 2017 13:07

youknowriad force-pushed the update/implement-block-registering branch 2 times, most recently from b445c38 to 2694251 Compare March 17, 2017 13:14

youknowriad commented Mar 17, 2017

View reviewed changes

aduth reviewed Mar 17, 2017

View reviewed changes

nylen force-pushed the update/implement-block-registering branch 2 times, most recently from 1e4b7b2 to c2e4c8c Compare March 17, 2017 17:13

aduth reviewed Mar 17, 2017

View reviewed changes

youknowriad commented Mar 17, 2017

View reviewed changes

aduth reviewed Mar 18, 2017

View reviewed changes

youknowriad force-pushed the update/implement-block-registering branch from e157c9a to 5904546 Compare March 20, 2017 09:46

Plugin: Fix test runner. Use babel-register instead of mocha-webpack

7482ebc

youknowriad mentioned this pull request Mar 21, 2017

Plugin: Fix test runner. Use babel-register instead of mocha-webpack #303

Merged

aduth added 2 commits March 21, 2017 11:21

Remove unused dependency

bce66e9

Avoid assigning globals

0f1ea2a

youknowriad mentioned this pull request Mar 21, 2017

Plugin: Adding a first version of block parser using a pegjs grammar #301

Merged

Add unregisterBlock and resulting improvements:

a512271

- No need for a separate `validateBlockSlug` function - More thorough and more isolated tests

Remove .tmp from .gitignore

3f3469e

This was from `mocha-webpack`, which is no longer used, so this entry is no longer needed.

Exclude node modules from Webpack JS rules

65066e1

Fix lint issues and correctly setup the npm test command

b95c775

Fix registering the example text block

e5f72ca

youknowriad merged commit ffd1cef into master Mar 22, 2017

youknowriad deleted the update/implement-block-registering branch March 22, 2017 14:09

aduth mentioned this pull request Mar 23, 2017

Run Mocha on Webpack-generated bundle #318

Merged

nylen mentioned this pull request May 3, 2017

Blocks: Propose a server side rendering implementation #586

Merged

jasmussen mentioned this pull request May 31, 2017

Implement front-end styling for blocks #963

Closed

aduth mentioned this pull request Apr 9, 2018

Build: Reference Lodash as an external module #5933

Merged

snyk-bot mentioned this pull request Jul 28, 2020

[Snyk] Upgrade markdownlint from 0.18.0 to 0.20.4 wpscholar/gutenberg#2

Open


		blocks[ namespace ][ block ] = Object.assign(
		{ name: block, namespace },

		@@ -1,5 +1,7 @@
		export { default as Editable } from './components/editable';

		const blocks = {};


		import * as blocks from '@wordpress/blocks';

		chai.use( dirtyChai );

Plugin: Implement block registering API #289

Plugin: Implement block registering API #289

Conversation

youknowriad commented Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nylen Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nylen commented Mar 17, 2017

youknowriad commented Mar 17, 2017

nylen commented Mar 17, 2017

youknowriad commented Mar 17, 2017 • edited Loading

nylen commented Mar 17, 2017 • edited Loading

nylen commented Mar 17, 2017

mtias commented Mar 17, 2017

nylen commented Mar 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nylen commented Mar 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nylen commented Mar 17, 2017

youknowriad commented Mar 17, 2017

youknowriad commented Mar 17, 2017 • edited Loading

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Mar 21, 2017

nylen commented Mar 21, 2017 • edited Loading

youknowriad commented Mar 22, 2017 • edited Loading

nylen commented Mar 22, 2017

youknowriad commented Mar 22, 2017

youknowriad commented Mar 22, 2017

aduth commented Mar 22, 2017

youknowriad commented Mar 22, 2017

youknowriad commented Mar 22, 2017

aduth commented Mar 22, 2017

youknowriad commented Mar 22, 2017

youknowriad commented Mar 17, 2017 •

edited

Loading

nylen Mar 17, 2017 •

edited

Loading

youknowriad commented Mar 17, 2017 •

edited

Loading

nylen commented Mar 17, 2017 •

edited

Loading

youknowriad Mar 17, 2017 •

edited

Loading

youknowriad commented Mar 17, 2017 •

edited

Loading

nylen commented Mar 21, 2017 •

edited

Loading

youknowriad commented Mar 22, 2017 •

edited

Loading