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

Extract Core Blocks to the npm packages folder #8287

Merged
merged 13 commits into from
Aug 10, 2018

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Jul 30, 2018

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

Testing instructionss

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

@youknowriad youknowriad added the npm Packages Related to npm packages label Jul 30, 2018
@youknowriad youknowriad self-assigned this Jul 30, 2018
@youknowriad youknowriad requested review from gziolo and a team July 30, 2018 10:39
@youknowriad youknowriad force-pushed the try/core-blocks-package branch 2 times, most recently from 57a6414 to ca40fba Compare July 30, 2018 10:47
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.
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

@hypest any thoughts?

'wp-blob',
'wp-blocks',
'wp-components',
'wp-compose',
'wp-data',
Copy link
Member

Choose a reason for hiding this comment

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

How it even worked before? 😅

"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",
Copy link
Member

Choose a reason for hiding this comment

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

Good one 👍

@@ -10,7 +10,7 @@ import { URL } from 'url';
import { times } from 'lodash';

const {
WP_BASE_URL = 'http://localhost:8889',
WP_BASE_URL = 'http://localhost:8888',
Copy link
Member

Choose a reason for hiding this comment

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

Is that intended change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no :P

import * as verse from './verse';
import * as video from './video';

export const registerCoreBlocks = () => {
Copy link
Member

Choose a reason for hiding this comment

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

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

registerBlockType( name, settings );
} );

setDefaultBlockName( paragraph.name );
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@gziolo gziolo Jul 31, 2018

Choose a reason for hiding this comment

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

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

@gziolo gziolo requested a review from hypest July 31, 2018 13:10
},
"main": "build/index.js",
"module": "build-module/index.js",
"dependencies": {
Copy link
Member

Choose a reason for hiding this comment

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

We need also an entry point for React Native:

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

@aduth
Copy link
Member

aduth commented Jul 31, 2018

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
Copy link
Contributor Author

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 🤷‍♂️

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.

This PR needs rebase.

@@ -121,6 +121,7 @@ zip -r gutenberg.zip \
gutenberg.php \
lib/*.php \
core-blocks/*/*.php \
packages/core-blocks/src/*/*.php \
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Core Blocks for the WordPress editor.
Copy link
Member

Choose a reason for hiding this comment

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

This would become:

Blocks library for the WordPress editor.

export const registerCoreBlocks = () => {
[
code,
more,
Copy link
Member

Choose a reason for hiding this comment

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

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

@import './text-columns/style.scss';
@import './video/style.scss';

.has-pale-pink-background-color {
Copy link
Member

Choose a reason for hiding this comment

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

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

@gziolo
Copy link
Member

gziolo commented Aug 6, 2018

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

gziolo commented Aug 6, 2018

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 gziolo force-pushed the try/core-blocks-package branch from ca40fba to c72500b Compare August 6, 2018 07:25
@gziolo
Copy link
Member

gziolo commented Aug 6, 2018

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

@youknowriad
Copy link
Contributor Author

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 gziolo force-pushed the try/core-blocks-package branch from c72500b to 8362a83 Compare August 9, 2018 15:42
@gziolo
Copy link
Member

gziolo commented Aug 9, 2018

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

@@ -203,8 +207,10 @@ function buildPackage( packagePath ) {
// Build js files individually.
jsFiles.forEach( ( file ) => buildJsFile( file, true ) );

// Build entire package scss.
buildPackageScss( packagePath );
const scssFiles = glob.sync( `${ srcDir }/*.scss` );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 gziolo requested review from vindl, gwwar and koke August 10, 2018 06:05
@youknowriad youknowriad force-pushed the try/core-blocks-package branch from 2606854 to 4e11e6d Compare August 10, 2018 09:30
@youknowriad youknowriad force-pushed the try/core-blocks-package branch from a83d982 to 1717c65 Compare August 10, 2018 10:00
@gziolo
Copy link
Member

gziolo commented Aug 10, 2018

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 gziolo added the [Type] Breaking Change For PRs that introduce a change that will break existing functionality label Aug 10, 2018
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.

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

Great work🏅

Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

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.

@@ -5,7 +5,7 @@ It turns out that writing the simplest possible block which contains only static
- [zgordon/gutenberg-course](https://github.com/zgordon/gutenberg-course) - a repository for Zac Gordon's Gutenberg Development Course
- [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.
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -1,6 +1,6 @@
# **core/editor**: The Editor’s Data

## Selectors
## Selectors
Copy link
Member

Choose a reason for hiding this comment

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

Trailing whitespace! 😱

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@tofumatt tofumatt 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 my comments have been addressed and this doesn't actually need any modification then.

:shipit:

@youknowriad youknowriad merged commit ad889ba into master Aug 10, 2018
@youknowriad youknowriad deleted the try/core-blocks-package branch August 10, 2018 13:06
@@ -16,11 +16,7 @@ import {
unstable__bootstrapServerSideBlockDefinitions, // eslint-disable-line camelcase
} from '@wordpress/blocks';
import { parse as grammarParse } from '@wordpress/block-serialization-spec-parser';

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be fixed in #8923

@@ -10,7 +10,7 @@ import { URL } from 'url';
import { times, castArray } from 'lodash';

const {
WP_BASE_URL = 'http://localhost:8889',
WP_BASE_URL = 'http://localhost:8888',
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Good catch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
npm Packages Related to npm packages [Type] Breaking Change For PRs that introduce a change that will break existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants