Blocks: Consider moving UI components out of /blocks folder

[gziolo]

Issue Overview

At the moment /blocks folder contains lots of smaller reusable components that are shared between many existing blocks. I found it very confusing where trying to locate one of such existing components. I expected to find it inside /components folder. If we would opt for moving those shared components to /components folder we could also fix this strange relative path that repeats in all blocks. It's not an issue at the moment, but as soon we will want to move one of the blocks outside of Gutenberg it might become an issue. It simply ties location of those components with the blocks forever. It also makes it harder to reuse those components in external blocks that we want to supper in the near future. I would like to propose to move those components inside /components folder and take advantage of @wordpress/components alias.

Current Behavior

Example file: https://github.com/WordPress/gutenberg/blob/da364e8a69ba288ac5cf154a9d4f97c7957faa34/blocks/library/gallery/block.js.

/**
 * WordPress dependencies
 */
import { Component } from '@wordpress/element';
import { __ } from '@wordpress/i18n';
import { mediaUpload } from '@wordpress/utils';
import { Dashicon, Toolbar, Placeholder, FormFileUpload } from '@wordpress/components';

/**
 * Internal dependencies
 */
import MediaUploadButton from '../../media-upload-button';
import InspectorControls from '../../inspector-controls';
import RangeControl from '../../inspector-controls/range-control';
import ToggleControl from '../../inspector-controls/toggle-control';
import SelectControl from '../../inspector-controls/select-control';
import BlockControls from '../../block-controls';
import BlockAlignmentToolbar from '../../block-alignment-toolbar';
import GalleryImage from './gallery-image';
import BlockDescription from '../../block-description';

Possible Solution

/**
 * WordPress dependencies
 */
import { Component } from '@wordpress/element';
import { __ } from '@wordpress/i18n';
import { mediaUpload } from '@wordpress/utils';
import {
	Dashicon,
	Toolbar,
	Placeholder,
	FormFileUpload,
	BlockAlignmentToolbar,
	BlockControls,
	BlockDescription,
	MediaUploadButton
} from '@wordpress/components';
import InspectorControls, {
	RangeControl,
	SelectControl,
	ToggleControl
} from '@wordpress/components/inspector-controls';

/**
 * Internal dependencies
 */
import GalleryImage from './gallery-image';

[aduth]

How do we define reusable? The components within blocks are very specific to the editor UI and would not provide much value in any other screens. Similarly, how do we handle components which are context-aware, such as BlockAutocomplete which must retrieve data of registered blocks. How would this render in a generic context? The approach there was to separate a context-agnostic Autocomplete component, on which BlockAutocomplete enhances with behaviors specific to blocks.

[youknowriad]

Not sure I agree with these changes, some of these components have strong dependencies and I don't think we should introduce these strong dependencies in the components module which is meant to be a reusable UI components module (without any WP dependency)

For example:

• BlockAlignmentToolbar, BlockControls both depend on being included in an Editor (they can not work outside)
• MediaUploadButton depend on the wp-media script to work

The inspector controls could be considered UI components though

[gziolo]

I noticed that my concerns will be partially addressed in #2795, where blocks folder is going to be merged into editor folder. I think it's a better solution than I proposed and perfectly aligns with what @aduth pointed out. I saw that you identified some standalone components that might have a chance to land into components folder. This is what I was exactly thinking about. I wasn't aware of all those subtle dependencies you both mentioned. Thanks for making it clear 🙇

Now let me elaborate more on the second part. We use all those relative paths everywhere, which is fine, but maybe we could borrow some patterns from Facebook and their custom module system called “Haste”. I recommend reading this part of React docs. They never use relative paths when importing. They explain that it makes it easy to move files to different folders and import them without worrying about relative paths. If you don't find it as easy to apply to Gutenberg, I'm happy to close this issue.

[aduth]

Re: Haste. Hmm, global uniqueness seems quite restrictive, though I assume in an application the size of Facebook, it would be fairly evident if this was an issue to scale, so clearly they've found some success. I do feel it would be quite confusing for the uninitiated, and Facebook themselves notes in the documentation you link that they may migrate to a more standard CommonJS / ES Modules for alignment with the community.

One of the original reasons for using relative paths was to help distinguish between external, "WordPress", and internal dependencies by path. Previously we did not have the @wordpress/ prefix for our package dependencies, and would sometimes even import from a nested path, so it was difficult to discern between foo/bar being internal to the current top-level directory, or part of a package dependency. Now that these packages are properly scoped and we've restricted path access, it may be more reasonable to consider setting each directory root as a modules (maybe as implemented in #386 😬 ).

[gziolo]

Now that these packages are properly scoped and we've restricted path access, it may be more reasonable to consider setting each directory root as a modules (maybe as implemented in #386 😬 ).

Yes, I like this idea a lot. This is what seemed like a perfect choice when I saw imports using @wordpress/components. I tracked down your changes and it looks like the reason why they were reverted is now resolved. I would be happy to bring this solution back.

Re: Haste. Hmm, global uniqueness seems quite restrictive, though I assume in an application the size of Facebook, it would be fairly evident if this was an issue to scale, so clearly they've found some success.

I only shared Facebook example to give a better context. I don't think we should introduce such restrictive rules. I like a lot what you initially introduced in #386. A full local import path also gives a bit better idea what are you really importing and makes it easy to move files to a different folder as you don't need to update import statements.

[gziolo]

It looks like it is as easy as including this Webpack plugin: ResolveEntryModulesPlugin, nice! I played a bit with it locally and it works like a charm. The only drawback I see here is that it further complicates IDE setup when you want to have a way to navigate to a file with a mouse click. @youknowriad what do you think about it? Would it make refactoring easier?

[youknowriad]

The relative paths don't bother me at all, in fact, sometimes I find them useful in distinguishing misplaced dependencies (between internal, WordPress and external deps). The most important thing is having a failed build when importing a wrong path. I have no strong opinion on dropping the relative paths or not

[gziolo]

Closing this one for now. I will open PR if I find it more important 😄