Packages: Move Custom CSS Properties to new `calypso-css-custom-properties` package #30933

ockham · 2019-02-21T13:20:52Z

Preparatory step to move Calypso's build system into an npm.
See #30772 (comment) (and conversation below) for background.

Questions/Notes

Do we like the name (calypso-css-custom-properties)? Yay? Nay?
I was originally going to move the entire assets/stylesheets/shared/ folder (and was going to call the package calypso-shared-style), but decided to keep the package as minimal as possible instead. Thoughts?
bin/build-packages doesn't work for styling-only packages. I think it's too rigid (might also be for different kinds of packages) -- my suggestion would be to make the build process each package's individual concern for greater flexibility, i.e. have bin/build-packages just run each package's npm run build. IIRC, lerna had a way to do that (but maybe only as part of bootstrap). Resolved, see Packages: Build individually #31112

Changes proposed in this Pull Request

Move Custom CSS Properties to new calypso-css-custom-properties package

Testing instructions

npm run distclean
npm start

Verify that Calypso runs as before, and that styling isn't broken.

matticbot · 2019-02-21T13:20:53Z

Test live: https://calypso.live/?branch=package/calypso-shared-style

assets/stylesheets/directly.scss

postcss.config.js

blowery · 2019-02-21T20:39:07Z

bin/build-packages doesn't work for styling-only packages. I think it's too rigid (might also be for different kinds of packages)

I totally agree it's too rigid. I think we could move to using a prepare or postinstall script in each package that uses a script from a build-tools package. Ordering might get a bit exciting there (are the build tools available when we want to run the build?)

postcss.config.js

ockham · 2019-02-22T22:43:44Z

Let's merge #30969 before this, rebase this PR, and apply this patch afterwards (background):

(Edit: done)

diff --git a/assets/stylesheets/directly.scss b/assets/stylesheets/directly.scss
index a82a2514a1..a6c71ca7b1 100644
--- a/assets/stylesheets/directly.scss
+++ b/assets/stylesheets/directly.scss
@@ -33,7 +33,7 @@
 // QUIT AND RESTART YOUR BROWSER NORMALLY WHEN YOU'RE DONE MAKING LOCAL CSS CHANGES.
 
 @import 'shared/mixins/mixins';
-@import '../../node_modules/@automattic/calypso-css-custom-properties/src/shared/colors';
+@import '~@automattic/calypso-css-custom-properties/src/shared/colors';
 @import 'shared/extends-forms';
 @import 'shared/forms';
 @import 'shared/typography';
diff --git a/assets/stylesheets/editor.scss b/assets/stylesheets/editor.scss
index 36d5d6fda0..048b452af8 100644
--- a/assets/stylesheets/editor.scss
+++ b/assets/stylesheets/editor.scss
@@ -1,5 +1,5 @@
-@import '../../node_modules/@automattic/calypso-css-custom-properties/src/shared/colors';
-@import '../../node_modules/@automattic/calypso-css-custom-properties/src/shared/color-schemes';
+@import '~@automattic/calypso-css-custom-properties/src/shared/colors';
+@import '~@automattic/calypso-css-custom-properties/src/shared/color-schemes';
 @import 'shared/typography';
 @import 'shared/mixins/breakpoints';
 
diff --git a/assets/stylesheets/emergent-paywall.scss b/assets/stylesheets/emergent-paywall.scss
index bfd605c5cd..9c85ea5615 100644
--- a/assets/stylesheets/emergent-paywall.scss
+++ b/assets/stylesheets/emergent-paywall.scss
@@ -28,7 +28,7 @@
 // QUIT AND RESTART YOUR BROWSER NORMALLY WHEN YOU'RE DONE MAKING LOCAL CSS CHANGES.
 
 @import 'shared/mixins/mixins';
-@import '../../node_modules/@automattic/calypso-css-custom-properties/src/shared/colors';
+@import '~@automattic/calypso-css-custom-properties/src/shared/colors';
 @import 'shared/extends-forms';
 @import 'shared/forms';
 @import 'shared/typography';
diff --git a/assets/stylesheets/shared/_utils.scss b/assets/stylesheets/shared/_utils.scss
index d4fabf9b6f..df14dbf1c8 100644
--- a/assets/stylesheets/shared/_utils.scss
+++ b/assets/stylesheets/shared/_utils.scss
@@ -1,6 +1,6 @@
 @import '../shared/functions'; // functions that we've used from Compass, ported over
 @import '../shared/functions/functions'; // sass functions for z-index, etc.
-@import '../../../node_modules/@automattic/calypso-css-custom-properties/src/shared/colors'; // import all of our wpcom colors
+@import '~@automattic/calypso-css-custom-properties/src/shared/colors'; // import all of our wpcom colors
 @import '../shared/typography'; // all the typographic rules, variables, etc.
 @import '../shared/variables'; // other variables
 @import '../shared/mixins/mixins'; // sass mixins for gradients, bordius radii, etc.
diff --git a/assets/stylesheets/shared/functions/_functions.scss b/assets/stylesheets/shared/functions/_functions.scss
index 1703cff79e..24684c455e 100644
--- a/assets/stylesheets/shared/functions/_functions.scss
+++ b/assets/stylesheets/shared/functions/_functions.scss
@@ -1,3 +1,3 @@
-@import '../../../../node_modules/@automattic/calypso-css-custom-properties/src/shared/functions/_hex-to-rgb';
+@import '~@automattic/calypso-css-custom-properties/src/shared/functions/_hex-to-rgb';
 @import 'overflow-gradient';
 @import 'z-index';
diff --git a/assets/stylesheets/style.scss b/assets/stylesheets/style.scss
index 32095d14c1..7c2c17b2ab 100644
--- a/assets/stylesheets/style.scss
+++ b/assets/stylesheets/style.scss
@@ -4,7 +4,7 @@
 // Shared
 @import 'shared/reset'; // css reset before the rest of the styles are defined
 @import 'shared/utils'; // utilities that are used by all CSS but don't produce any code
-@import '../../node_modules/@automattic/calypso-css-custom-properties/src/shared/color-schemes'; // import color schemes
+@import '~@automattic/calypso-css-custom-properties/src/shared/color-schemes'; // import color schemes
 @import 'shared/animation'; // all UI animation
 @import 'shared/forms'; // form styling
 @import 'shared/welcome'; // welcome messages

ockham · 2019-02-22T22:52:44Z

bin/build-packages doesn't work for styling-only packages. I think it's too rigid (might also be for different kinds of packages)

I totally agree it's too rigid. I think we could move to using a prepare or postinstall script in each package that uses a script from a build-tools package. Ordering might get a bit exciting there (are the build tools available when we want to run the build?)

Ordering is even going to be interesting when packaging these things -- if this package wanted to provide a script that relies on a build-tools package, I'd have to create the latter first. Which might not be an easy feat because of the CSS build stack relying on custom properties.

Thinking of breaking the vicious cycle by filing a PR to simply have each package provide a build script for now that doesn't rely on any TBD packages.

ockham · 2019-02-25T20:37:13Z

packages/calypso-css-custom-properties/src/shared/_colors.scss

@@ -1,5 +1,5 @@
 // The Muriel Color Palette
-@import '~color-studio/dist/color-variables';
+@import '../../node_modules/color-studio/dist/color-variables';


I essentially reverted this change here. The reason is that node-sass-package-importer isn't great at transitivity:

Calypso doesn't just import calypso-css-custom-properties' main entry point (dist/custom-properties.css), but continues to also import this file (shared/colors) which in turn has the import of color-studio's color-variables file I'm modifying here.

Now if we used the node-sass-package-importer-based import notation (@import '~color-studio/dist/color-variables'), any consumer of that package would look for that path in its own node_modules/ directory -- rather than (transitively) in calypso-sdk-custom-properties. It follows that:

Any consumer of calypso-sdk-custom-properties (including Calypso!) will either have to install color-studio itself (by adding it to its own package.json, or

calypso-css-custom-properties would need to add another build step to provide a file where this import is resolved.

Or we just make the import path explicit like I'm doing here, which seems like a lot less hassle 😄 it's calypso-css-custom-properties's only import of an external SASS file anyway.

I think you have to use the tilde syntax here. The location of node_modules relative to this file isn't guaranteed.

Hmm, sure about that? (Or maybe I'm really getting my wires crossed here.)

This (i.e. the src/shared/_colors.scss file in the calypso-css-custom-properties package) is referring to that package's own node_modules/ directory here (whose relative location should thus be known and invariant).

The issue is with consuming projects that import this file: if they see a @import '~color-studio/dist/color-variables' in it, they'll look for that file in their own node_modules/ rather than in calypso-css-custom-properties's -- and they won't fit there (unless it's also installed there -- but requiring them to would kinda suck, see my previous comment).

(This is kinda backed up by my experiments with both notations 😬 )

(My generalized takeaway would BTW be -- Don't use ~ SASS imports in npm modules (libraries) but only in apps that aren't published/imported by other projects (b/c of lack of transitivity))

This (i.e. the src/shared/_colors.scss file in the calypso-css-custom-properties package) is referring to that package's own node_modules/ directory here (whose relative location should thus be known and invariant).

Try running npm dedupe on this branch. color-studio will get hoisted to the root node_modules and the node_modules under color-studio will be removed. Yay for npm's non-determinism. Another way to get there would be to add color-studio as a dependency to the root package.json, then run npm run update-deps.

The issue is with consuming projects that import this file: if they see a @import '~color-studio/dist/color-variables' in it, they'll look for that file in their own node_modules/ rather than in calypso-css-custom-properties's

Consuming projects should depend on the package, which depends on color-studio, so it should be present somewhere in the node_modules tree. If consuming projects are using this file on it's own without the supporting npm infrastructure ... I dunno.

This (i.e. the src/shared/_colors.scss file in the calypso-css-custom-properties package) is referring to that package's own node_modules/ directory here (whose relative location should thus be known and invariant).

Try running npm dedupe on this branch. color-studio will get hoisted to the root node_modules and the node_modules under color-studio will be removed.

Okay, can repro -- that makes the build fail 🙁

Yay for npm's non-determinism. Another way to get there would be to add color-studio as a dependency to the root package.json, then run npm run update-deps.

The issue is with consuming projects that import this file: if they see a @import '~color-studio/dist/color-variables' in it, they'll look for that file in their own node_modules/ rather than in calypso-css-custom-properties's

Consuming projects should depend on the package, which depends on color-studio, so it should be present somewhere in the node_modules tree. If consuming projects are using this file on it's own without the supporting npm infrastructure ... I dunno.

So to recap: Starting from a clean slate (npm run distclean && npm ci):

With
@import '../../node_modules/color-studio/dist/color-variables';: after npm dedupe, npm start fails because color-studio has been hoisted to the top-level node_modules/. (It works if npm dedupe isn't called, though.)

With @import '~color-studio/dist/color-variables';: npm start fails, because it's looking for color-studio in the top-level node_modules/ (but it hasn't been installed explicitly there, nor hoisted. npm dedupe makes it work).

So how do we get out of this mess? Just install color-studio explicitly at the top level?

With @import '~color-studio/dist/color-variables';: npm start fails, because it's looking for color-studio in the top-level node_modules/ (but it hasn't been installed explicitly there, nor hoisted. npm dedupe makes it work).

In this case, who's doing the import? Something in assets/stylesheets or something in calypso-css-custom-properties? If the former, yeah, we'd have to add a dependency on color-studio to the root for resolution to work. If the latter, ... I would expect that to work and would need to dig more.

That's an interesting observation. So, with the following directory structure:

node_modules/color-studio/index.scss color-schemes/node_modules/color-studio/index.scss color-schemes/index.scss

and contents of color-schemes/index.scss:

@import '~color-studio';

the import gets resolved to node_modules/color-studio instead of color-schemes/node_modules/color-studio? That's not how the resolution algorithm should work and it's a bug in node-sass-package-importer.

The node-sass importer function gets the file path from where the import is performed as the second parameter and it should use it to resolve:

function importer( url, prev ) { if ( ! url.startsWith( '~' ) ) { return null; // not our business to resolve this path } return resolve.sync( url.slice( 1 ), { baseDir: dirname( prev ) } ); }

What node-sass-package-importer does instead is that is passes process.cwd() as baseDir.

The other package that @blowery suggested in #30933 (comment), https://github.com/anarh/node-sass-import, doesn't have this bug. But it doesn't support the ~ prefix 🙁

Not a reason to block this PR, the __color-studio solution is OK, but would be nice to file some issues and come back some time later.

Thanks for digging into that @jsnajdr !

Filed maoberlehner/node-sass-magic-importer#188

blowery · 2019-02-26T20:57:50Z

I did some experimenting with how npm handles lifecycle hooks with file refs. It looks like we could add a prepare script to each package that needs to do a build and have it use a root level devDependency, which can itself be a package.

So root package.json would have prepare: "npx lerna run prepare --stream" and each package that needs to do a build could use prepare: "calypso-module-builder" and packages that need to do something fancier could do the fancy thing.

One nice thing here is that lerna respects package dependency order when running the script across all the packages.

sirreal · 2019-03-06T10:01:49Z

I also flipped it back to spaces.

This shrinkwrap has tabs at 1015c77 😱

There was another new conflict, I've rebased and cleaned up some of the messier tabs/spaces commits.

A new shrinkwrap generated at the HEAD uses spaces.

sirreal · 2019-03-06T10:06:44Z

package.json

@@ -249,7 +247,7 @@
    "clean": "npm run -s clean:build && npm run -s clean:devdocs && npm run -s clean:public && npm run -s clean:packages",
    "clean:build": "npx rimraf build server/bundler/*.json .babel-cache",
    "clean:devdocs": "npx rimraf server/devdocs/search-index.js server/devdocs/proptypes-index.json server/devdocs/components-usage-stats.json",
-    "clean:packages": "npx rimraf packages/*/dist",
+    "clean:packages": "npx lerna run clean",


Should we --stream this like build?

sure. it just changes the output format. i don't have a strong preference between standard and stream.

sirreal · 2019-03-06T11:20:57Z

Tests are still passing after rebase, I'll smoke test and plan to land this if things look good.

ockham · 2019-03-07T09:09:35Z

packages/calypso-css-custom-properties/bin/build-custom-properties-css.js

+
+if ( ! existsSync( dirname( OUTPUT_FILE ) ) ) {
+	mkdirSync( dirname( OUTPUT_FILE ), { recursive: true } );
+}


I didn't add an exitsSync check since my reading (and testing) of the recursive: true arg was that it worked even if the path existed already 😬 (But I should've commented to that effect.)

ockham · 2019-03-07T09:14:17Z

Thank you so much for carrying this over the finish line, @sirreal, and thanks @blowery and @jsnajdr for valuable comments! ❤️

ockham added [Status] In Progress Build Packages labels Feb 21, 2019

ockham self-assigned this Feb 21, 2019

ockham force-pushed the package/calypso-shared-style branch 2 times, most recently from fc8b1e1 to 6120300 Compare February 21, 2019 13:53

ockham changed the title ~~Packages: Move assets/stylesheets/shared/ to new calypso-shared-style package~~ Packages: Move assets/stylesheets/shared/ to new calypso-shared-style package Feb 21, 2019

ockham changed the title ~~Packages: Move assets/stylesheets/shared/ to new calypso-shared-style package~~ Packages: Move Custom CSS Properties to new calypso-css-custom-properties package Feb 21, 2019

ockham commented Feb 21, 2019

View reviewed changes

assets/stylesheets/directly.scss Outdated Show resolved Hide resolved

ockham commented Feb 21, 2019

View reviewed changes

postcss.config.js Outdated Show resolved Hide resolved

ockham added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Feb 21, 2019

ockham requested review from simison, sirreal and a team February 21, 2019 18:51

ockham marked this pull request as ready for review February 21, 2019 18:51

blowery reviewed Feb 21, 2019

View reviewed changes

postcss.config.js Outdated Show resolved Hide resolved

ockham force-pushed the package/calypso-shared-style branch from 44ec31e to 583ce6a Compare February 22, 2019 20:31

ockham added [Status] Blocked / Hold and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 22, 2019

ockham force-pushed the package/calypso-shared-style branch from d7a68d2 to 3d368cf Compare February 25, 2019 20:06

ockham commented Feb 25, 2019

View reviewed changes

ockham added [Status] In Progress and removed [Status] Blocked / Hold labels Feb 25, 2019

ockham force-pushed the package/calypso-shared-style branch from 0fbff62 to 2e575b0 Compare February 27, 2019 16:54

ockham and others added 17 commits March 6, 2019 10:43

Shut up eslint

0b6cf2f

Add package-lock.json

4141963

Update README

ba2d25a

Add package eslint

1e753c9

Use standard sass imports by copying required file

6ddaffd

Remove package-lock.json

d501480

Update deps

d1479de

Make scripts more consistent and robust

5e8baa2

Build packages before css

afdb29f

Add clean scripts

04c0c79

Drop build steps from package readme

43e0adc

Use npx lerna

502b56c

Update new package name

0ef1c22

Update README

6df1215

Rename files to calypso-color-schemes

9eb4956

update name of the css file in package

79b88b8

Update deps

82ec0a7

sirreal force-pushed the package/calypso-shared-style branch from 1015c77 to 82ec0a7 Compare March 6, 2019 10:00

sirreal added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 6, 2019

sirreal reviewed Mar 6, 2019

View reviewed changes

sirreal removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 6, 2019

sirreal merged commit 32c6921 into master Mar 6, 2019

sirreal deleted the package/calypso-shared-style branch March 6, 2019 11:24

matticbot removed the [Status] Ready to Merge label Mar 6, 2019

ockham commented Mar 7, 2019

View reviewed changes

ockham mentioned this pull request Mar 8, 2019

node-sass-magic-importer: Transitive imports broken maoberlehner/node-sass-magic-importer#188

Open

Aurorum mentioned this pull request Mar 8, 2019

DevDocs: Fix Broken Colour Link #31311

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Packages: Move Custom CSS Properties to new `calypso-css-custom-properties` package #30933

Packages: Move Custom CSS Properties to new `calypso-css-custom-properties` package #30933

ockham commented Feb 21, 2019 •

edited

Loading

matticbot commented Feb 21, 2019

blowery commented Feb 21, 2019

ockham commented Feb 22, 2019 •

edited

Loading

ockham commented Feb 22, 2019

ockham Feb 25, 2019 •

edited

Loading

blowery Feb 26, 2019

ockham Feb 26, 2019

ockham Feb 26, 2019

blowery Feb 28, 2019

ockham Feb 28, 2019

blowery Mar 1, 2019

jsnajdr Mar 5, 2019

blowery Mar 5, 2019

ockham Mar 8, 2019

blowery commented Feb 26, 2019 •

edited

Loading

sirreal commented Mar 6, 2019

sirreal Mar 6, 2019

blowery Mar 6, 2019

sirreal commented Mar 6, 2019 •

edited

Loading

ockham Mar 7, 2019 •

edited

Loading

ockham commented Mar 7, 2019

Packages: Move Custom CSS Properties to new calypso-css-custom-properties package #30933

Packages: Move Custom CSS Properties to new calypso-css-custom-properties package #30933

Conversation

ockham commented Feb 21, 2019 • edited Loading

Questions/Notes

Changes proposed in this Pull Request

Testing instructions

matticbot commented Feb 21, 2019

blowery commented Feb 21, 2019

ockham commented Feb 22, 2019 • edited Loading

ockham commented Feb 22, 2019

ockham Feb 25, 2019 • 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

blowery commented Feb 26, 2019 • edited Loading

sirreal commented Mar 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirreal commented Mar 6, 2019 • edited Loading

ockham Mar 7, 2019 • edited Loading

Choose a reason for hiding this comment

ockham commented Mar 7, 2019

Packages: Move Custom CSS Properties to new `calypso-css-custom-properties` package #30933

Packages: Move Custom CSS Properties to new `calypso-css-custom-properties` package #30933

ockham commented Feb 21, 2019 •

edited

Loading

ockham commented Feb 22, 2019 •

edited

Loading

ockham Feb 25, 2019 •

edited

Loading

blowery commented Feb 26, 2019 •

edited

Loading

sirreal commented Mar 6, 2019 •

edited

Loading

ockham Mar 7, 2019 •

edited

Loading