Extract Core Blocks to the npm packages folder by youknowriad · Pull Request #8287 · WordPress/gutenberg

youknowriad · 2018-07-30T10:39:03Z

This PR tries to extract the core blocks package partially to the npm packages folder.
It lives these two blocks out:

core/html: Because it relies on the CodeEditor component which is not available in the npm components package (due to lazy loading issues, related Tips: status should be user-based not browser-based #8052 Babel Plugin Async Load: Lazy load react components when necessary #8017)
core/freeform: I doubt it'll ever be possible to port this to the npm package because it relies on wp.oldEditor. The only way to do so would be to make sure the old editor is also available as an npm package. Not sure if possible cc @iseulde

Testing instructionss

Try playing with some blocks and make sure the tests pass.

gziolo · 2018-07-31T12:46:36Z

core-blocks/index.js

+import * as verse from '../packages/core-blocks/src/verse';
+import * as video from '../packages/core-blocks/src/video';
+
+// The freeform block can't be moved to the "npm" packages folder because it requires the wp.oldEditor global.


Let's hope that order of imports doesn't matter anymore :)

gziolo · 2018-07-31T12:47:54Z

core-blocks/index.native.js

-		registerBlockType( name, settings );
-	} );
-};
+import '../packages/core-blocks/src';


We probably can remove this file and ensure that Mobile loads the package directly.

@hypest any thoughts?

gziolo · 2018-07-31T12:48:57Z

lib/client-assets.php

 			'wp-blocks',
 			'wp-components',
 			'wp-compose',
+			'wp-data',


How it even worked before? 😅

gziolo · 2018-07-31T12:49:35Z

package.json

-		"fixtures:clean": "rimraf \"core-blocks/test/fixtures/*.+(json|serialized.html)\"",
-		"fixtures:server-registered": "docker-compose run -w /var/www/html/wp-content/plugins/gutenberg --rm wordpress ./bin/get-server-blocks.php > core-blocks/test/server-registered.json",
+		"fixtures:clean": "rimraf \"test/integration/full-content/fixtures/*.+(json|serialized.html)\"",
+		"fixtures:server-registered": "docker-compose run -w /var/www/html/wp-content/plugins/gutenberg --rm wordpress ./bin/get-server-blocks.php > test/integration/full-content/server-registered.json",


Good one 👍

gziolo · 2018-07-31T12:53:19Z

test/e2e/support/utils.js


 const {
-	WP_BASE_URL = 'http://localhost:8889',
+	WP_BASE_URL = 'http://localhost:8888',


Is that intended change?

gziolo · 2018-07-31T12:55:29Z

packages/core-blocks/src/index.js

+import * as verse from './verse';
+import * as video from './video';
+
+export const registerCoreBlocks = () => {


So we have it duplicated in here but without two problematic blocks. Interesting.

gziolo · 2018-07-31T12:56:32Z

packages/core-blocks/src/index.js

+		registerBlockType( name, settings );
+	} );
+
+	setDefaultBlockName( paragraph.name );


We don't call setUnknownTypeHandlerName. Will it cause some issues?

Ironically, we can't set it to both HTML and Freeform :(

gziolo · 2018-07-31T13:12:19Z

packages/core-blocks/package.json

+	},
+	"main": "build/index.js",
+	"module": "build-module/index.js",
+	"dependencies": {


We need also an entry point for React Native:

"react-native": "src/index",

aduth · 2018-07-31T14:09:24Z

The only way to do so would be to make sure the old editor is also available as an npm package.

Would it be fair to say that the component could be used if the global (or some appropriately-named alternative) is available? Or some way to provide the implementation at runtime?

youknowriad · 2018-07-31T14:33:29Z

Would it be fair to say that the component could be used if the global (or some appropriately-named alternative) is available? Or some way to provide the implementation at runtime?

Yes, that's true (I prefer avoiding the global though, a setter at runtime possible) but I kind of hope all WP JS scripts become packages at some point 🤷‍♂️

gziolo

This PR needs rebase.

gziolo · 2018-08-06T06:33:14Z

bin/build-plugin-zip.sh

 	gutenberg.php \
 	lib/*.php \
 	core-blocks/*/*.php \
+	packages/core-blocks/src/*/*.php \


I think we agreed to rename this stuff to blocks-library or something along those lines. /cc @mtias

gziolo · 2018-08-06T06:34:45Z

packages/core-blocks/README.md

@@ -0,0 +1,21 @@
+# Core Blocks
+
+Core Blocks for the WordPress editor.


This would become:

Blocks library for the WordPress editor.

gziolo · 2018-08-06T06:36:06Z

packages/core-blocks/src/index.native.js

+export const registerCoreBlocks = () => {
+	[
+		code,
+		more,


I think, it needs rebase. Paragraph was exposed recently.

gziolo · 2018-08-06T06:37:03Z

packages/core-blocks/src/style.scss

+@import './text-columns/style.scss';
+@import './video/style.scss';
+
+.has-pale-pink-background-color {


Should we put colors in its own file? I'm also not quite sure why they are located in this package?

gziolo · 2018-08-06T06:42:09Z

Would it be fair to say that the component could be used if the global (or some appropriately-named alternative) is available? Or some way to provide the implementation at runtime?

Yes, that's true (I prefer avoiding the global though, a setter at runtime possible) but I kind of hope all WP JS scripts become packages at some point 🤷‍♂️

Another way of tackling it is to use the same approach we did for wp.media dependent code. Provide a fallback which renders nothing and replaces it with a WP specific implementation on runtime. I think we could improve the integration part and for the time we have so many dependencies on WP globals, we could expose @wordpress/compat package which would contain all hooks which bring in those functionalities. As soon as we migrate chunks of code we could remove those hooks.

gziolo · 2018-08-06T07:11:40Z

Find a way to declare multiple CSS entry points per package to build multiple CSS files per package (theme, editor and style)

Let's tackle this one separately and focus on exposing blocks to npm so people could start experimenting.

gziolo · 2018-08-06T07:28:23Z

I rebased with master and tested it locally. So far everything looks good 👍

youknowriad · 2018-08-06T07:52:28Z

Let's tackle this one separately and focus on exposing blocks to npm so people could start experimenting.

I feel this is important for npm people to be able to show the block styles properly

gziolo · 2018-08-09T15:44:11Z

8362a83 tries to solve the issue with the styles prepared for npm publish. It might not fully work, but in general creates files properly :)

youknowriad · 2018-08-09T15:51:59Z

bin/packages/build.js


-	// Build entire package scss.
-	buildPackageScss( packagePath );
+	const scssFiles = glob.sync( `${ srcDir }/*.scss` );


So if I understand properly, this considers all root level files in as entry points. I personally don't like this convention because in some packages a src/style.scss may import and src/something.scss and we'd still have a unique entry point.

I think I'd prefer having the current behavior: the src/style.scss file convention as a fallback and explicit entry points (maybe a custom property in the package.json in case there are multiple)

That said, I can be ok with the current behavior temporarily, I'd like if we document it somewhere though.

Last thing, I think this breaks "watching" sass files, we probably have to rebuild all the files and not only the style.scss one if a sass file changes.

Personally, I would go further and glob for all scss files, the same way we do it for JS files. The next step would be a bit different. Instead of transpiling all of them as it happens for JS, we would group them based on the file name and produce one .css, e.g. all style.scss files from one package would get merged into build-style/style.css.

// Exclude deceitful source-like files, such as editor swap files. const isSourceFile = ( filename ) => { return /.\.(js|scss)$/.test( filename ); };

Last thing, I think this breaks "watching" sass files, we probably have to rebuild all the files and not only the style.scss one if a sass file changes.

execSync( `${ BUILD_CMD } ${ files.join( ' ' ) }`, { stdio: [ 0, 1, 2 ] } );

Yes, correct. We need to improve this one somehow. It probably would make sense to implement the grouping I described which would ultimately solve the latter.

gziolo · 2018-08-10T10:37:47Z

I think we need to provide deprecation strategy for core-blocks because it exposes registerCoreBlocks method. Ideally, we should get rid of it at all ...

gziolo

In my testing, everything works like before. All changes I can think of were applied. We have deprecation in place.
Let's 🚢

Great work🏅

tofumatt

I have a few tweaks but I'll make them myself. Only question is do we have an issue for that 3.8 deprecation? I know we'll probably remember it, but an issue in the milestone might be nice as it doesn't look like our linter will catch it.

tofumatt · 2018-08-10T11:38:27Z

docs/blocks/generate-blocks-with-wp-cli.md

 - [ahmadawais/create-guten-block](https://github.com/ahmadawais/create-guten-block) - A zero-configuration developer toolkit for building WordPress Gutenberg block plugins

-It might be also a good idea to browse the folder with [all core blocks](https://github.com/WordPress/gutenberg/tree/master/core-blocks) to see how they are implemented.
+It might be also a good idea to browse the folder with [all core blocks](https://github.com/WordPress/gutenberg/tree/master/packages/block-library/src) to see how they are implemented.


While we're updating the name, calling these "built-in" or "default" blocks might be nicer than "core blocks". Thoughts?

tofumatt · 2018-08-10T11:38:50Z

docs/data/data-core-editor.md

 # **core/editor**: The Editor’s Data

-## Selectors
+## Selectors 


Trailing whitespace! 😱

This is generated, we might have an issue in the build tool

tofumatt · 2018-08-10T11:40:13Z

lib/client-assets.php

+		filemtime( gutenberg_dir_path() . 'build/block-library/index.js' ),
+		true
+	);
+	// Remove it with 3.8.0 release.


Can we tag this so it's findable or have an issue for it referenced here? Anything so this doesn't get lost.

I think it's fine, I think we don't need the comment at all because the deprecated call on the frontend will be caught and when we remove it, the module will be empty.

Ah, okay, gotcha! That makes sense, I'll ignore it then. 👍

tofumatt

Looks like my comments have been addressed and this doesn't actually need any modification then.

ellatrix · 2018-08-13T13:38:51Z

test/integration/full-content/full-content.js

@@ -16,11 +16,7 @@ import {
 	unstable__bootstrapServerSideBlockDefinitions, // eslint-disable-line camelcase
 } from '@wordpress/blocks';
 import { parse as grammarParse } from '@wordpress/block-serialization-spec-parser';
-


@youknowriad I think this file no longer runs as part of npm run test-unit? Is that intentional? Additionally require( '../../packages/editor/src/hooks' ); won't work if forced to run.

Good catch, It was not intentional, I just moved it to the integration tests folder but apparently this folder needs a .spec in the file name to run tests.

It should be fixed in #8923

tofumatt · 2018-08-17T15:43:38Z

test/e2e/support/utils.js


 const {
-	WP_BASE_URL = 'http://localhost:8889',
+	WP_BASE_URL = 'http://localhost:8888',


In this huge diff I missed this change, but we shouldn't be doing this. We have a separate test environment for a reason 😄

Sorry that was a debug change :), I would have expected the tests to fail because of this change :P

Yeah, I'm a bit surprised they didn't, but actually the point is to have a fresh state that's always reset for the tests… it might be that because the tests on Travis are fresh no matter what it worked. I'll see about adding a check for this that at least warns us in my fix?

Good catch.

youknowriad added the npm Packages Related to npm packages label Jul 30, 2018

youknowriad self-assigned this Jul 30, 2018

youknowriad requested review from a team and gziolo July 30, 2018 10:39

youknowriad force-pushed the try/core-blocks-package branch 2 times, most recently from 57a6414 to ca40fba Compare July 30, 2018 10:47

gziolo reviewed Jul 31, 2018

View reviewed changes

gziolo requested a review from hypest July 31, 2018 13:10

gziolo reviewed Jul 31, 2018

View reviewed changes

youknowriad mentioned this pull request Aug 2, 2018

Add watch support for stylesheets in packages #8380

Merged

4 tasks

gziolo reviewed Aug 6, 2018

View reviewed changes

gziolo force-pushed the try/core-blocks-package branch from ca40fba to c72500b Compare August 6, 2018 07:25

gziolo force-pushed the try/core-blocks-package branch from c72500b to 8362a83 Compare August 9, 2018 15:42

youknowriad commented Aug 9, 2018

View reviewed changes

gziolo requested review from gwwar, koke and vindl August 10, 2018 06:05

youknowriad force-pushed the try/core-blocks-package branch from 2606854 to 4e11e6d Compare August 10, 2018 09:30

youknowriad and others added 5 commits August 10, 2018 10:46

Extract Core Blocks to the npm packages folder

05ad41a

Core blocks: Adjustment after rebase with master

e4cc457

Build: Add support for all scss files in build-style directory

81cae91

Build: Fix output path for styles generated for packages

837b3e2

Fix sass files after rebase

4fab1ad

youknowriad added 2 commits August 10, 2018 10:46

Fix watching SASS files

1f36b25

Rename core-blocks block-library

1717c65

youknowriad force-pushed the try/core-blocks-package branch from a83d982 to 1717c65 Compare August 10, 2018 10:00

youknowriad added 2 commits August 10, 2018 11:21

Fix linting issues

e0a23d7

Update docs

e6f6da7

gziolo added the [Type] Breaking Change For PRs that introduce a change that will break existing functionality label Aug 10, 2018

youknowriad and others added 4 commits August 10, 2018 12:11

Deprecate Core Blocks

a23dc80

Update index.js

4b93b84

Update client-assets.php

348156c

Update client-assets.php

29370a5

gziolo approved these changes Aug 10, 2018

View reviewed changes

tofumatt reviewed Aug 10, 2018

View reviewed changes

tofumatt approved these changes Aug 10, 2018

View reviewed changes

youknowriad merged commit ad889ba into master Aug 10, 2018

youknowriad deleted the try/core-blocks-package branch August 10, 2018 13:06

chrisvanpatten mentioned this pull request Aug 12, 2018

404, missing build/core-blocks/style.css #8886

Closed

Shelob9 mentioned this pull request Aug 13, 2018

Packages: Publish all modules as independent npm packages #3955

Closed

19 tasks

ellatrix reviewed Aug 13, 2018

View reviewed changes

youknowriad mentioned this pull request Aug 13, 2018

Test: Ensure the full content integration test is run #8923

Merged

jahvi mentioned this pull request Aug 16, 2018

Blocks: Tag Cloud #7875

Merged

4 tasks

tofumatt reviewed Aug 17, 2018

View reviewed changes

aduth mentioned this pull request Sep 17, 2018

Option to disable default styles to allow AMP compatible theme implementation (solved by changing wp_deregister_style('wp-block-library'); to wp_deregister_style('wp-core-blocks');) #9896

Closed

Soean mentioned this pull request Oct 9, 2018

Update: Readme - Full post content test fixtures #10432

Merged

		@@ -0,0 +1,21 @@
		# Core Blocks

		Core Blocks for the WordPress editor.

Conversation

youknowriad commented Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth commented Jul 31, 2018

Uh oh!

youknowriad commented Jul 31, 2018

Uh oh!

gziolo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo commented Aug 6, 2018

Uh oh!

gziolo commented Aug 6, 2018

Uh oh!

gziolo commented Aug 6, 2018

Uh oh!

youknowriad commented Aug 6, 2018

Uh oh!

gziolo commented Aug 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo commented Aug 10, 2018

Uh oh!

gziolo left a comment

Choose a reason for hiding this comment

Uh oh!

tofumatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tofumatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad commented Jul 30, 2018 •

edited

Loading

gziolo Jul 31, 2018 •

edited

Loading