Export scss vars as json during compilation

[weltenwort]

This PR enhances the script scripts/compile-scss.js to export also extract and export the sass variables from each theme into a corresponding json file in dist/eui_theme_${THEME}.json. The values in this file are in a format suitable for use in styled-components themes to keep the scss files as a single source of truth.

It also converts the script to use async I/O to improve parallelism.

The export of ready-to-use themes could be considered in a subsequent step.

[cjcenizal]

@weltenwort Thanks for doing this! I won't be able to review this so I'm tagging in @mattapperson and @snide.

[sorenlouv]

I know you didn't write this code, but since we are at it, perhaps replace all filename logic with path.parse. You can get it mostly with a one-liner, and it'll work cross platform (I don't think Windows likes the forward slashes):

const { name: baseFileName } = path.parse(file);

[sorenlouv]

Maybe a catch at the bottom, so errors doesn't get swallowed?

[sorenlouv]

nit: I think this would benefit from async/await.

[mattapperson]

I am curious if it is better to save as a JSON file or a js file exporting an object. JSON will have parsing overhead at runtime as webpack will bundle as a string then JSON.parse on each require (at least last time I checked)

[sorenlouv]

This is really great. Thanks for doing this!! 👍

[weltenwort]

Thanks for the very valuable feedback, @sqren!

I have flattened the Promise chains a bit using aync/await and split it up into several functions. (Too bad that node 8 does not yet support for await ... of.) I also removed the superfluous shelljs dependency and try to handle all paths in a cross-platform manner using path.parse and path.join. Additionally, the script now isolates errors while processing each file.

Let me know what you think.

[mattapperson]

@weltenwort I am curious if it is better to save as a JSON file or a js file exporting an object. JSON will have parsing overhead at runtime as webpack will bundle as a string then JSON.parse on each require (at least last time I checked)

[weltenwort]

I thought about that. But as soon as it's a js file it would be subject to the "is this an ES module or a commonjs module" lookup dance. So we would have to write it to both src (for ES modules) and lib (for commonjs), which I don't like (especially the "writing to src" part).

[sorenlouv]

lgtm

[mattapperson]

@weltenwort Well, yea writing to SRC is bad. So what if we then create an export on EUI for the JSON file that pre-parses it one time, preventing multiple parsing?

[mattapperson]

Another added perk to exporting the configs is it gives it an API rather then just a file. This way we can change the implementation without breaking compatibility if we need to down the road

[weltenwort]

I agree that exposing a javascript interface would be nice, but...

That adds the assumption that any consumer of the commonjs or es module can import json. AFAIK that is the case for node, webpack and browserify, but I don't know if that's sufficient.

We could put the json import in something like src/themes/index.js without re-exporting it in src/index.js, so it's up to the consumer to execute that code path. 🤔

[snide]

This will certainly work, and as the consumers of the variables you three will probably know best about what you need so I'll leave it to you if this solution works for you.

All that said, when actually composing our styles on a day to day basis, you can see @cchaos and I are using mixins pretty heavily. Bringing over the variables is just part of it. Without the mixins, you'll likely be duplicating a lot of the work and eventually going off pattern for larger projects. A simple example would be dealing with shadows. Right now those are all mixins. You'll likely need to rebuild them by hand (and again, could be different per theme!).

I'd eventually like to use something like https://github.com/4Catalyzer/css-literal-loader/blob/master/README.md#experimental-component-api which seems much more wholistic with its import, allowing you to directly import sass into your JS (mixins included). Unfortunately I haven't had time to really build out a decent test of it, but in concept it seems to cover more of what we'd want.

There's no reason we can't move there eventually, and since the vars shouldn't change this would hopefully just be an import switch when we do. If this is a quick stepping stone to unblock you all I'm fine with it, but thought I'd bring it up in case you have time.

One question though. If I change the name of a var in Sass now, would you consider that a breaking change that needs documentation? It would likely break your consumption no? It doesn't happen too much on the core global_styling variables, but likely happens on the inner modules from time to time (For example, we've rewritten the forms SCSS a few times now).

Beyond all that commentary, if you all decide this is the way you want to handle the JS vars, then please add some documentation for how this works. Specifically I'd expect...

• Some comments in the Readme / wiki
• Some comments in the how to consume EUI docs.
• Possibly a documentation page in Utilities that shows an example.

Again, all of these are short term solutions. Long term I think the whole system will be in some form of CSS in JS system with a more contained, modularized approach. I've tried to write the SCSS that way so that when the transition comes, it should be easy to port over without too much detangling!

[weltenwort]

thanks for sharing your thoughts, @snide

breaking changes: That is already the case with just scss. Renaming variables and mixins would break any scss code that imports it, even when it's statically compiled. If we say "please re-use the variables and mixins when writing new styles", we make them part of the public api.

mixins: Somehow "importing" the scss mixins will be difficult since they can contain complex logic. A solution would probably be so complicated that I would question the benefits of using it.

Looking at css-literal-loader, it seems that its approach is to extract the style snippets from js files and statically compile them at build time. That is different from what most css-in-js solutions do, because it prevents most kinds of dynamic style changes.

So the question seems to be whether we can live without reusing the mixins in components outside of EUI. @sqren, having used styled-components in production while trying to match EUI styling, did you miss the ability to access the sass mixins so far?

[sorenlouv]

So the question seems to be whether we can live without reusing the mixins in components outside of EUI. @sqren, having used styled-components in production while trying to match EUI styling, did you miss the ability to access the sass mixins so far?

I didn't miss sass mixins, since mixins feel very natural in js (example from the styled-components docs):

import { truncate } from '../style-utils';

// Make this div truncate the text with an ellipsis
const Box = styled.div`
  ${ truncate('250px') }
  background: papayawhip;
`;

If a lot of work has been put into creating sass mixins, it will of course take time to redo that in js (if needed). But until now I haven't really missed EUI mixins. I mostly use EUI's react components and configure them through props.
I use styled-components for custom details, and minor things that EUI doesn't have to care about.

So from my POV we can get by without the mixins - at least for now.

[snide]

Cool. Like I said, you all are the audience most aware of the needs. I do like the looks of that library (it does support mixins somehow) but I'm not hung up on it.

Seems like an OK solution for the moment.

[mattapperson]

@felix-lessoer I am fine with adding it to src/themes/index.js

alternatively though, what if we add something like the solution outlined here webpack/webpack#6572. That way the JSON is just included in the bundle as an object?

[weltenwort]

I would like to avoid cluttering the build process with more and more loaders... especially since the "bundle" is only one of three ways to consume EUI. Kibana is probably using the es-module variant straight from src.

(also, wrong Felix 😉)

[mattapperson]

But it’s not adding the need for another loader. If you have the loader you get better perf then just loading the json and parsing at runtime... graceful degradation. We can still put the access to the api inside arc/themes/index.js or whatever you like for that

[mattapperson]

I am curious if it is better to save as a JSON file or a js file exporting an object. JSON will have parsing overhead at runtime as webpack will bundle as a string then JSON.parse on each require (at least last time I checked)

[weltenwort]

I have added a small documentation segment with an example for using the exported json files with `styled-components'. Furthermore, I added a simple typescript definition to satisfy the compiler.

I'd like to defer the following items to a follow-up PR:

• Add a small JS api to access the variables for each theme (as proposed by @mattapperson)
• Add a more complete typescript definition to enabled typed theme

If the reviewers could take another look at these latest changes to ensure it doesn't change the approval they graciously gave? 😉

[mattapperson]

WOOT! Thanks for this @weltenwort!