Packages: Move the components module partially to the packages folder #7640

youknowriad · 2018-06-29T16:10:48Z

This PR makes the components module a generic npm ready package.

It doesn't use @wordpress/utils anymore. I copied the last remaining function (Sometimes copy pasting small function is better than figuring out a complex setup)
I added a buildStyle function to the build script of the packages to generate a CSS file per package if needed. The package should have a src/style.scss as an entry point.
I kept the CodeEditor component outside of the components npm package because it's not ready: It needs lazyloading (babel plugin) and a dependency towards @wordpress/code-editor
I did some manual tweaks to the dashicon/index.js file, I know @aduth planned to work on it, so we need to sync here.

tofumatt

It doesn't use @wordpress/utils anymore. I copied the last remaining function (Sometimes copy pasting small function is better than figuring out a complex setup)

That's because @wordpress/utils isn't published, right? Could you add a note about the duplicated function maybe? I agree with you about it not being worth a complex set up, but if it were a matter of just including another module dependancy available from NPM I'd say we should move to that once possible.

I have some questions about the change of style imports used but largely looks good.

tofumatt · 2018-06-29T16:15:32Z

bin/packages/build.js

@@ -14,12 +14,16 @@ const glob = require( 'glob' );
 const babel = require( 'babel-core' );
 const chalk = require( 'chalk' );
 const mkdirp = require( 'mkdirp' );
+const sass = require( 'node-sass' );
+const postcss = require( 'postcss' );
+const deasync = require( 'deasync' );


That's a tough one to read… might be nicer to import as deAsync 🤷‍♂️

Even if it's nonsense, we should be strictly consistent in our camelcasing. If this were a module named de-async, I'd agree deAsync would be reasonable. Same reason we [should] use import classnames from 'classnames'; and not import classNames from 'classnames';.

tofumatt · 2018-06-29T16:16:53Z

bin/packages/build.js

+	const builtSass = sass.renderSync( {
+		file: styleFile,
+		includePaths: [ path.resolve( __dirname, '../../edit-post/assets/stylesheets' ) ],
+		data: '@import "colors"; @import "breakpoints"; @import "variables"; @import "mixins"; @import "animations";@import "z-index";' +


Could this be… a multiline template string or something? To increase readability? If not it's fine and it's easy to grok after a second, but it's just a bit harsh 😄

Could this be… a multiline template string or something?

Maybe just an array mapping (or reduce) result:

[ 'colors', 'breakpoints', 'variables', 'mixins', 'animations', 'z-index', ].map( ( imported ) => `import "${ imported }";` ).join( '' );

tofumatt · 2018-06-29T16:17:33Z

bin/packages/build.js

+	};
+
+	const result = deasync( postCSSSync )();
+	fs.writeFileSync( outputFile, result.css, () => true );


What's the no-op return true function for?

I don't know :) copy pasted from the docs of postcss :)

Since this will be confusing to the hypothetical future maintainer, we should do one of:

Explain why it exists in an inline comment

Remove it

tofumatt · 2018-06-29T16:19:25Z

components/index.js

-export { default as Tooltip } from './tooltip';
-export { default as TreeSelect } from './tree-select';
-export { createSlotFill, Slot, Fill, Provider as SlotFillProvider } from './slot-fill';
+export * from '../packages/components/src';


Update: I see it's basically just moved, haha.

tofumatt · 2018-06-29T16:21:00Z

packages/components/src/form-token-field/index.js

@@ -13,7 +13,6 @@ import { Component } from '@wordpress/element';
 /**
 * Internal dependencies
 */
-import './style.scss';


Why aren't these importing their local stylesheets anymore? Are they being magically loaded in from elsewhere?

tofumatt · 2018-06-29T16:23:23Z

packages/components/src/date-time/style.scss

@@ -14,7 +14,8 @@ $datepicker__muted-color: #ccc;
 $datepicker__navigation-disabled-color: lighten($datepicker__muted-color, 10%);
 $datepicker__border-color: $light-gray-500;

-@import '~react-datepicker/src/stylesheets/datepicker';
+// Lerna hoist packages, so can't reference with ~


Should these comments be "hoists packages"?

We no longer use lerna bootstrap --hoist, I'm wondering if it is still the case.

tofumatt · 2018-06-29T16:24:52Z

packages/components/src/style.scss

@@ -0,0 +1,34 @@
+@import './autocomplete/style.scss';


Oh, I see. So is every component's style loaded here now instead of in the component? That strikes me as less follow-able.

can we generate this file using glob?

tofumatt · 2018-06-29T16:27:29Z

Why is:

I added a buildStyle function to the build script of the packages to generate a CSS file per package if needed. The package should have a src/style.scss as an entry point.

Needed? It makes it harder to look at a component's index.js file and see everything it imports. I always really liked the pattern of importing stylesheets from a component.

youknowriad · 2018-06-29T16:58:23Z

Needed? It makes it harder to look at a component's index.js file and see everything it imports. I always really liked the pattern of importing stylesheets from a component.

It is needed because we need a way to build a stylesheet per package for npm consumption and we don't use webpack to build the packages because we don't need bundling.

It's probably possible to use webpack but it's a change too big for this PR and personally, I perfer this new setup because it shows clearly what are our outputs.

I always really liked the pattern of importing stylesheets from a component.

I also do but in our case, I don't think it was good because we're not really using the imported value we just extract and concat all the stylesheets together. I think it can mislead people to think that the styles are scoped while in fact they are not. I think it's a good pattern to use with CSS modules or any CSS in JS solution but in our case, I think it's a bit misleading.

tofumatt · 2018-06-29T17:40:49Z

Ah, that explanation helps a lot. Thanks. 👍

aduth · 2018-07-10T15:19:51Z

bin/packages/build.js

+	const builtSass = sass.renderSync( {
+		file: styleFile,
+		includePaths: [ path.resolve( __dirname, '../../edit-post/assets/stylesheets' ) ],
+		data: '@import "colors"; @import "breakpoints"; @import "variables"; @import "mixins"; @import "animations";@import "z-index";' +


Could this be… a multiline template string or something?

Maybe just an array mapping (or reduce) result:

[ 'colors', 'breakpoints', 'variables', 'mixins', 'animations', 'z-index', ].map( ( imported ) => `import "${ imported }";` ).join( '' );

aduth · 2018-07-10T15:21:42Z

bin/packages/build.js

+	};
+
+	const result = deasync( postCSSSync )();
+	fs.writeFileSync( outputFile, result.css, () => true );


Since this will be confusing to the hypothetical future maintainer, we should do one of:

Explain why it exists in an inline comment

Remove it

aduth · 2018-07-10T15:23:20Z

bin/packages/build.js

+	const styleFile = path.resolve( packagePath, SRC_DIR, 'style.scss' );
+	const outputFile = path.resolve( packagePath, BUILD_DIR.style, 'style.css' );
+	mkdirp.sync( path.dirname( outputFile ) );
+	const builtSass = sass.renderSync( {


I know it's convenient for development, but a build script responsible for generating many distinct modules seems particularly write for an async implementation, not sync.

yes, true. I think this could be handled separately as it has implications on the whole build script and not only the styles build script.

aduth · 2018-07-10T15:24:13Z

bin/packages/post-css-config.js

@@ -0,0 +1,64 @@
+module.exports = [
+	require( '../../packages/postcss-themes/src/index.js' )( {


What if src/index.js contains code not supported by Node native? We're assumed to be able to use modern features there, and this may subtly break in the future.

We can use @wordpress/postcss-themes since we started using local packages.

gziolo · 2018-07-11T08:49:25Z

packages/components/src/date-time/style.scss

@@ -14,7 +14,8 @@ $datepicker__muted-color: #ccc;
 $datepicker__navigation-disabled-color: lighten($datepicker__muted-color, 10%);
 $datepicker__border-color: $light-gray-500;

-@import '~react-datepicker/src/stylesheets/datepicker';
+// Lerna hoist packages, so can't reference with ~


We no longer use lerna bootstrap --hoist, I'm wondering if it is still the case.

gziolo · 2018-07-11T08:50:58Z

packages/components/src/style.scss

@@ -0,0 +1,34 @@
+@import './autocomplete/style.scss';


can we generate this file using glob?

gziolo · 2018-07-11T08:55:53Z

webpack.config.js

-					require( 'autoprefixer' ),
-					require( 'postcss-color-function' ),
-				],
+				plugins: require( './bin/packages/post-css-config' ),


Is this related to components package in any way? I'm wondering if this is something mandatory to be included in this PR. I'm not against, it's rather me trying to understand how it relates.

that's only because we're using the same config here and in the build script of the packages. It can be removed from webpack once all packages are moved to the lerna setup.

Cool, it makes sense 👍

gziolo · 2018-07-11T08:57:27Z

packages/components/package.json

+		"@wordpress/keycodes": "^1.0.0-alpha.1",
+		"@wordpress/i18n": "1.1.0",
+		"@wordpress/is-shallow-equal": "1.0.2",
+		"classnames": "2.2.5",


Should we bundle classnames into element or mark it as external dependency and convert it into its own WP module?

I don't know if it's really an issue, it's 1kb unminified.

Right, it probably would make more sense with other packages as noted here: #7640 (comment)

gziolo · 2018-07-11T08:58:26Z

packages/components/package.json

+	"main": "build/index.js",
+	"module": "build-module/index.js",
+	"dependencies": {
+		"@wordpress/a11y": "1.0.6",


After we upgraded to Lerna 3.x, this should use local packages:

"@wordpress/a11y": "file:../a11y", "@wordpress/api-request": "file:../api-request", // etc.

gziolo · 2018-07-11T09:00:54Z

packages/components/package.json

+		"clipboard": "1.7.1",
+		"dom-scroll-into-view": "1.2.1",
+		"http-build-query": "0.7.0",
+		"lodash": "4.17.5",


"lodash": "4.17.10" - we now use a different version of lodash.

gziolo · 2018-07-11T09:04:52Z

packages/components/package.json

+		"react-click-outside": "2.3.1",
+		"react-color": "2.13.4",
+		"react-datepicker": "1.4.1",
+		"rememo": "3.0.0",


I noticed that not only classnames is used in a few packages, but also:
memize, rememo or uuid. We probably should create a vendor bundle and put them together there to optimize code send to the browser.

gziolo · 2018-07-11T09:07:55Z

packages/components/README.md

+Components
+==========
+
+This packages includes a library of generic React components to be used for creating common UI elements shared between screens and features of the WordPress dashboard.


React components - should we call them WordPress components instead?

gziolo · 2018-07-11T09:16:58Z

That's because @wordpress/utils isn't published, right? Could you add a note about the duplicated function maybe? I agree with you about it not being worth a complex set up, but if it were a matter of just including another module dependancy available from NPM I'd say we should move to that once possible.

We don't want to publish @wordpress/utils at all but rather split it into smaller more focused packages. I agree that we should at least add a comment. I was also wondering if we could refactor code in a way where data returned from the REST endpoint could be mapped using this helper so we had only one place where it is used - that is the selector.

gziolo · 2018-07-17T08:20:28Z

bin/packages/post-css-config.js

@@ -0,0 +1,64 @@
+module.exports = [
+	require( '../../packages/postcss-themes/src/index' )( {


I'm wondering if we can use @wordpress/postcss-themes instead?

Let me give it a try :)

gziolo · 2018-07-17T11:22:42Z

Doing sanity check for chunks sent to the browser using npm run build:

Before

                              Asset       Size  Chunks                    Chunk Names
       ./build/core-blocks/index.js    188 KiB      20  [emitted]         coreBlocks
          ./build/viewport/index.js   24.3 KiB       0  [emitted]         viewport
           ./build/plugins/index.js   24.5 KiB       2  [emitted]         plugins
          ./build/keycodes/index.js   17.9 KiB       3  [emitted]         keycodes
  ./build/is-shallow-equal/index.js   6.39 KiB       4  [emitted]         isShallowEqual
              ./build/i18n/index.js   27.4 KiB       5  [emitted]         i18n
     ./build/html-entities/index.js  986 bytes       6  [emitted]         htmlEntities
             ./build/hooks/index.js   19.3 KiB       7  [emitted]         hooks
           ./build/element/index.js   42.5 KiB       8  [emitted]         element
         ./build/dom-ready/index.js  741 bytes       9  [emitted]         domReady
               ./build/dom/index.js   22.8 KiB      10  [emitted]         dom
        ./build/deprecated/index.js   10.8 KiB      11  [emitted]         deprecated
              ./build/date/index.js    192 KiB      12  [emitted]         date
              ./build/data/index.js   62.5 KiB      13  [emitted]         data
         ./build/core-data/index.js   54.6 KiB      14  [emitted]         coreData
           ./build/compose/index.js   32.4 KiB      15  [emitted]         compose
              ./build/blob/index.js  909 bytes      16  [emitted]         blob
       ./build/api-request/index.js   21.2 KiB      17  [emitted]         apiRequest
              ./build/a11y/index.js   5.32 KiB      18  [emitted]         a11y
               ./build/nux/index.js   23.8 KiB      19  [emitted]         nux
         ./build/shortcode/index.js   16.2 KiB       1  [emitted]         shortcode
         ./build/edit-post/index.js   91.1 KiB      21  [emitted]         editPost
             ./build/utils/index.js     41 KiB      22  [emitted]         utils
            ./build/editor/index.js    343 KiB      23  [emitted]  [big]  editor
        ./build/components/index.js    500 KiB      24  [emitted]  [big]  components
            ./build/blocks/index.js    293 KiB      25  [emitted]  [big]  blocks
      ./build/core-blocks/style.css     15 KiB      20  [emitted]         coreBlocks
./build/core-blocks/edit-blocks.css   22.5 KiB      20  [emitted]         coreBlocks
      ./build/core-blocks/theme.css   1.24 KiB      20  [emitted]         coreBlocks
       ./build/components/style.css   73.6 KiB      24  [emitted]         components
           ./build/editor/style.css   64.5 KiB      23  [emitted]         editor
        ./build/edit-post/style.css   29.5 KiB      21  [emitted]         editPost
              ./build/nux/style.css   1.64 KiB      19  [emitted]         nux
          ./build/nux/style-rtl.css   1.64 KiB      19  [emitted]         nux
  ./build/core-blocks/style-rtl.css     15 KiB      20  [emitted]         coreBlocks
    ./build/edit-post/style-rtl.css   29.5 KiB      21  [emitted]         editPost
       ./build/editor/style-rtl.css   64.3 KiB      23  [emitted]         editor
   ./build/components/style-rtl.css   73.5 KiB      24  [emitted]         components

./build/core-blocks/edit-blocks-rtl.css 22.5 KiB 20 [emitted] coreBlocks
./build/core-blocks/theme-rtl.css 1.24 KiB 20 [emitted] coreBlocks

After

                             Asset       Size  Chunks                    Chunk Names
       ./build/core-blocks/index.js    188 KiB      20  [emitted]         coreBlocks
          ./build/viewport/index.js   24.3 KiB       0  [emitted]         viewport
           ./build/plugins/index.js   24.5 KiB       2  [emitted]         plugins
          ./build/keycodes/index.js   17.9 KiB       3  [emitted]         keycodes
  ./build/is-shallow-equal/index.js   6.39 KiB       4  [emitted]         isShallowEqual
              ./build/i18n/index.js   27.4 KiB       5  [emitted]         i18n
     ./build/html-entities/index.js  986 bytes       6  [emitted]         htmlEntities
             ./build/hooks/index.js   19.3 KiB       7  [emitted]         hooks
           ./build/element/index.js   42.5 KiB       8  [emitted]         element
         ./build/dom-ready/index.js  741 bytes       9  [emitted]         domReady
               ./build/dom/index.js   22.8 KiB      10  [emitted]         dom
        ./build/deprecated/index.js   10.8 KiB      11  [emitted]         deprecated
              ./build/date/index.js    192 KiB      12  [emitted]         date
              ./build/data/index.js   62.5 KiB      13  [emitted]         data
         ./build/core-data/index.js   54.6 KiB      14  [emitted]         coreData
           ./build/compose/index.js   32.4 KiB      15  [emitted]         compose
              ./build/blob/index.js  909 bytes      16  [emitted]         blob
       ./build/api-request/index.js   21.2 KiB      17  [emitted]         apiRequest
              ./build/a11y/index.js   5.32 KiB      18  [emitted]         a11y
               ./build/nux/index.js   23.8 KiB      19  [emitted]         nux
         ./build/shortcode/index.js   16.2 KiB       1  [emitted]         shortcode
         ./build/edit-post/index.js   91.1 KiB      21  [emitted]         editPost
             ./build/utils/index.js     41 KiB      22  [emitted]         utils
            ./build/editor/index.js    343 KiB      23  [emitted]  [big]  editor
        ./build/components/index.js    499 KiB      24  [emitted]  [big]  components
            ./build/blocks/index.js    293 KiB      25  [emitted]  [big]  blocks
      ./build/core-blocks/style.css     15 KiB      20  [emitted]         coreBlocks
./build/core-blocks/edit-blocks.css   22.5 KiB      20  [emitted]         coreBlocks
      ./build/core-blocks/theme.css   1.24 KiB      20  [emitted]         coreBlocks
       ./build/components/style.css   73.6 KiB      24  [emitted]         components
           ./build/editor/style.css   64.5 KiB      23  [emitted]         editor
        ./build/edit-post/style.css   29.5 KiB      21  [emitted]         editPost
              ./build/nux/style.css   1.64 KiB      19  [emitted]         nux
          ./build/nux/style-rtl.css   1.64 KiB      19  [emitted]         nux
  ./build/core-blocks/style-rtl.css     15 KiB      20  [emitted]         coreBlocks
    ./build/edit-post/style-rtl.css   29.5 KiB      21  [emitted]         editPost
       ./build/editor/style-rtl.css   64.3 KiB      23  [emitted]         editor
   ./build/components/style-rtl.css   73.5 KiB      24  [emitted]         components

./build/core-blocks/edit-blocks-rtl.css 22.5 KiB 20 [emitted] coreBlocks
./build/core-blocks/theme-rtl.css 1.24 KiB 20 [emitted] coreBlocks

gziolo

In my testing everything works great. Let's merge it and observe how it works.

Some further improvements we should look into:

collect all the .scss files using script rather than maintain the list manually
INCLUDE_PACKAGES in buld:packages needs some rework. It would be nice to keep this list in the config or even build it based on package.json file using whitelist from devDependencies.

youknowriad added the npm Packages Related to npm packages label Jun 29, 2018

youknowriad self-assigned this Jun 29, 2018

youknowriad requested review from a team and mtias June 29, 2018 16:10

tofumatt reviewed Jun 29, 2018

View reviewed changes

Shelob9 mentioned this pull request Jul 10, 2018

Packages: Publish all modules as independent npm packages #3955

Closed

19 tasks

aduth reviewed Jul 10, 2018

View reviewed changes

gziolo reviewed Jul 11, 2018

View reviewed changes

youknowriad force-pushed the add/components-package branch 3 times, most recently from 4d1e43d to f1c1efe Compare July 16, 2018 15:59

mmtr mentioned this pull request Jul 16, 2018

Proof of concept: render a Gutenberg component in Calypso DevDocs Automattic/wp-calypso#26088

Closed

4 tasks

youknowriad force-pushed the add/components-package branch 2 times, most recently from e3afd8f to 6ec5b63 Compare July 16, 2018 16:30

gziolo added the [Priority] High Used to indicate top priority items that need quick attention label Jul 17, 2018

gziolo reviewed Jul 17, 2018

View reviewed changes

youknowriad added 8 commits July 17, 2018 10:39

Preparing the components package to be reusable

a5ee024

Adding Stylesheet build support to the packages

2a56b49

Fix postcss config (the wordpress dependency is not working properly)

cfef9ae

Tweaks to the build styles script

44773e5

Fix components package.json deps

5ae9343

Tweak README

770ebf4

fix unit tests

2f8c9a3

Fix the scroll lock style import

19ea586

gziolo and others added 3 commits July 17, 2018 10:39

Components: Add a few tweaks to the configuration of components package

afc887f

Fix the components docs config

a5fd1b9

Remove useless/moved file

6e115e9

youknowriad force-pushed the add/components-package branch from 3b8babd to 6e115e9 Compare July 17, 2018 09:41

Adding the deprecated package to wp-components dependencies

330a4cf

gziolo approved these changes Jul 17, 2018

View reviewed changes

gziolo added this to the 3.3 milestone Jul 17, 2018

youknowriad merged commit 6928e41 into master Jul 17, 2018

youknowriad deleted the add/components-package branch July 17, 2018 11:39

This was referenced Jul 19, 2018

Packages: Extract @wordpress/nux package #8016

Merged

Build: Improve the way styles built for packages are consumed #8067

Closed

jorgefilipecosta mentioned this pull request Jul 20, 2018

Fix: Regression on the color picker width #8095

Merged

Soean mentioned this pull request Jul 22, 2018

Block drop zone has no background color #8115

Closed

		@@ -0,0 +1,64 @@
		module.exports = [
		require( '../../packages/postcss-themes/src/index.js' )( {

Packages: Move the components module partially to the packages folder #7640

Packages: Move the components module partially to the packages folder #7640

Conversation

youknowriad commented Jun 29, 2018

tofumatt 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

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

Choose a reason for hiding this comment

tofumatt commented Jun 29, 2018

youknowriad commented Jun 29, 2018

tofumatt commented Jun 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad Jul 12, 2018 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Jul 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Jul 17, 2018 • edited Loading

Before

After

gziolo left a comment

Choose a reason for hiding this comment

youknowriad Jul 12, 2018 •

edited

Loading

gziolo commented Jul 17, 2018 •

edited

Loading