Sass explicit includes

[BPScott]

WHY are these changes introduced?

TL;DR:

Instead of writing components like:

.ActionList {
  padding: spacing(tight) 0;
}

where the spacing function (and others like color etc) are magically imported at build time, we should explicitly import the files that define that function:

@import '../../styles/common';

.ActionList {
  padding: spacing(tight) 0;
}

This will simplify our build config, and the build config of any project that wishes to consume Polaris components such as our own storybook an uxpin integrations, projects that use sewing-kit, and sister libraries like online-store-ui.

---

For a while I've been lamenting that polaris needs a chunk of custom config to enable running it from the esnext build / from source (so that people can gain treeshakable css or sidestep the need for a build step). If we could reduce the amount of config needed then we make it easier to integrate into external tools (e.g. storybook, uxpin, playroom), for external parties to consume our esnext build (e.g. people using create-react-app could take advantage of the esnext build) and configuration in sewing-kit and our own build could be simplified.
This is most easily seen in our storybook webpack config where we have to jump through hoops to expose our "global" scss functions/mixins - but the same config exists in sewing-kit and every tool that we'd wish to integrate.

• We use sass-resources-loader to make the functions/mixins defined in src/styles/foundations.scss and src/styles/foundations available to all our components without needing an explicit import
• We define import paths on sass-loader because we use a bunch of absolute imports instead of relative imports between the files in src/styles.

A few months ago I took a Frideations day to explore "What would it take to reduce/remove that config" and here's what I came up with. I left it to rot for a while but recent conversations with @tsov have made me think it's worth looking at again as they are looking at simplifying sewing-kit's webpack config to make it easier for other non-polaris libraries to be included (he's working on a pattern library for online-store-web and related products).

WHAT is this pull request doing?

This PR eliminates the need for import path configuration and the sass-resources-loader plugin (check the storybook webpack file and our build scripts for the simplifications this allows). This is a step towards simplifying our build configuration, thus making it easier to integrate with other as outlined above.

It does this by:

• changing all absolute imports to relative imports, e.g. @import 'foundation/utilities' in src/styles/foundation.scss becomes @import './foundation/utilities'. Thus removing the need for import paths
• explicitly importing the common file required in every component's scss file instead of implictly importing foundation and shared at build time.

Having the explicit imports is going to be the contentious bit - it's a bit more boilerplate compared to the implictness we were used to, but it does help show where certain functions/mixins come from instead of being magically always available.

[beefchimi]

Left one (in my opinion) critical comment about relative vs absolute import paths.

Beyond that - I like the explicitness of importing our sass dependencies per file - but I'm also concerned that the tooling isn't quite on par with what we all expect in JS/TS-land. When using something in JS/TS, we can automatically get the import path added to the top of our file. We also get some helpful path autocompletion when typing out import paths. I'm assuming we have none of that in Sass-ville?

[beefchimi]

I reeeaally don't like this being a relative path. Developers should not have to fight with SASS errors / failed builds as they in/decrement ../ trying to get to the right path.

Why are these not all absolute?

[kaelig]

See my comment below – I'd agree with you in an app context, but when building a library there is a case for optimizing the consumer experience, which sometimes comes at the expense of the contributor's experience.

[tsov]

Removing the sass-resource-loader would be amazing! Meaning consumers will have to explicitly import foundation/shared resources in order to use functions/mixins/variables (e.g. color()) and we can remove the special setup inside sewing kit.

I would agree with @beefchimi , having relative paths for foundation imports inside component sass files seems annoying and does not improve Developer Experience, however using absolute paths may require more setup for the consumer.

I see 2 options to enable this feature:

1. Keep the includePaths, so we can resolve foundation/colors (which means the consumer needs to resolve these)
2. Rewrite those absolute import paths to relative import paths during the build step (so the package will always contain relative paths)

I personally favour option 2.

I tried to unpick foundations into individual files but that just got annoying so I made it so you just import the whole thing.

I think that is fair, it's harder to do in retro than writing a new component. Either way is fine IMO.

Do you think this seems like a worthwhile idea?

Absolutely 💯 will reduce headaches for the consumer for sure. Also, as mentioned before, the sewing kit setup for polaris can be reduced.

How do you feel about the overhead of multiple explicit imports for shared items instead of a single "@import '../.../../shared'"? Would you prefer a single import for those at the expense of sometimes importing mixins you don't need.

As mentioned above I'm fine with either, as long as there is an explicit import in the first place.

[kaelig]

We also get some helpful path autocompletion when typing out import paths. I'm assuming we have none of that in Sass-ville?

Does this help? https://marketplace.visualstudio.com/items?itemName=mrmlnc.vscode-scss

Not sure this extension works well with "Keep the includePaths, so we can resolve foundation/colors".

---

As a library, I think Polaris should aim for compatibility towards zero-config builds, and optimize consumer efficiency, rather than optimizing developer/contributor efficiency.

For this reason, I'd support keeping the default includePaths, even if it means having to write a lot of ../../ in import statements.

[BPScott]

I'm also concerned that the tooling isn't quite on par with what we all expect in JS/TS-land. When using something in JS/TS, we can automatically get the import path added to the top of our file. We also get some helpful path autocompletion when typing out import paths. I'm assuming we have none of that in Sass-ville?

We don't get "reference a function and the import line gets added", but I'm using the "path-intellisense" VSCode plugin (as suggested in this package's .vscode configs) and I can do @import '../ and it offers up the rest of the path for me so that's something. You can cmd+click on import paths and they should take you to that defined file (once this VSCode bug is fixed). Note that this only works for relative paths, this doesn't work for absolute paths as the sass extension wouldn't know to resolve foundations to ../../foundations because you can't teach it about include paths.

I reeeaally don't like this being a relative path. Developers should not have to fight with SASS errors / failed builds as they in/decrement ../ trying to get to the right path. Why are these not all absolute?

If these paths are absolute then it means that consuming applications (sewing-kit, storybook, uxpin, playroom etc) had to add the styles folder into their sass-loader's include paths (e.g. in sewing-kit, in storybook.

There's currently potential for weirdness in projects that use sewing-kit as both polaris's styles folder and the app's src/styles folders are both added to the inclide path and that means an app can't have a src/styles/foundations.scss as that would end up being imported instead of polaris's css if you do @import 'foundations';, and thus your scss compilation would go boom.

Using relative paths within Polaris removes the need for polaris's src/styles folder to be added to the include path of consuming applications which removes that potential for ambiguity. @tsov spun up an alternative branch where we write absolute imports, but then on build they are rewritten to be relative, thus removing the need for some consuming apps to not need the include path, but I think that adds too much complexity at build time and the increased fragility and increase in debugging difficulty is not worth developers saving a few characters and occasionally having to check another file or add ../s till autocomplete tells them the right path.

[BPScott]

I used to use https://marketplace.visualstudio.com/items?itemName=mrmlnc.vscode-scss but have since uninstalled it as it didn't seem to place nice with VSCode's Live Share stuff (it pegged the CPU at 100% because it couldn't find remote files).

[BPScott]

Removing the sass-resource-loader would be amazing! Meaning consumers will have to explicitly import foundation/shared resources in order to use functions/mixins/variables (e.g. color()) and we can remove the special setup inside sewing kit.

While this totally enables removing sass-resource-loader from sewing-kit I'm a little wary of doing so. Lots have projects have grown up with the expectation that they can write color() and it work without imports. Removing sass-resource-loader From SK would break them all so we'd want some kind of way to opt into the cleaner behaviour rather than forcing it on everyone immediately. That said this PR does change the state of the world from "SK has to do this because that's what how polaris's own files expect" to "SK does this because it chooses to" so we unblock that new world.

I would agree with @beefchimi , having relative paths for foundation imports inside component sass files seems annoying and does not improve Developer Experience, however using absolute paths may require more setup for the consumer.

I see 2 options to enable this feature:

1. Keep the includePaths, so we can resolve foundation/colors (which means the consumer needs to resolve these)
2. Rewrite those absolute import paths to relative import paths during the build step (so the package will always contain relative paths)

As you've probably guessed from my prior reply I dislike both of these options. Keeping includePaths goes against a core goal of this PR idea as doing so means we can't remove any config in consumers. Writing absolute paths then rewriting them at build time increases build complexity on our end and I think the developer experience of easier debugging that if it goes wrong is worth having to type a few more '../../'s. Oh and also "transforms as a build step" would kill the aspiration of having a live editing of polaris components within a consuming application as such a thing would need to read our source code instead of built output.

[ry5n]

Does our tooling avoid duplicate CSS when importing the same file multiple times? That used to require some workarounds, if I recall.

[BPScott]

Does our tooling avoid duplicate CSS when importing the same file multiple times?

The contents of the shared and foundation files are all mixins and functions, they contain no css selectors, thus there is nothing to duplicate.

[tsov]

The more I think about this the more I agree with the approach 👍 It's about the consumer in the end, and this change would eliminate a lot of config for consumers. Just to give an example for the snowflake effect: @beefchimi team is working on a component library (very much inspired by polaris), that builds online store specific UI on top of polaris.
In order for that component library to consume polaris and also export its own component and foundational sass, we need to create an insane setup for sewing kit, because of all includePaths and the resource loader. Here the config for the consuming app:

https://github.com/Shopify/online-store-web/blob/master/config/online-store-ui.ts

Most of the changes (probably more than 60%) in that file is sass related config to suit polaris. These would be obsolete if polaris maintains relative paths only

[BPScott]

ping @Shopify/polaris so they see this and offer up some feedback (even if it's a "yep seems ok to me"). I'd like to continue to explore getting this working but I'd like some more explicit buy-in from the team.

[danrosenthal]

I like that this makes things more flexible for consumers, and that it results in a simpler webpack config, but I'm pretty out of my element thinking about those things.

In terms of the implications on how we write scss, this would represent a serious breaking change. Many of our consumers rely on being able to just use these sass variables and functions without an import. That means a lot of changes, not only in our own code, but also in a consumer's code. That makes this a change for a major release (v4 or later), if it's a change we want to make.

Somewhat related, but this feels like an opportunity to rethink the surface area of our SASS API. If we exposed less to consumers, I would feel more confident this was the right change.

Related: https://github.com/Shopify/polaris-react-deprecated/issues/1242

It would be good to revisit that issue above, and think about whether there are opportunities here for v4 or later major releases.

[dleroux]

From slack:

We could reduce the boilerplate by having a single import '../../../shared' instead of importing each shared file individually

That would be nicer.

The contents of the shared and foundation files are all mixins and functions, they contain no css selectors, thus there is nothing to duplicate.

We'd probably to ensure this stay like this somehow because even 1 selector can add up.

[BPScott]

In terms of the implications on how we write scss, this would represent a serious breaking change. Many of our consumers rely on being able to just use these sass variables and functions without an import. That means a lot of changes, not only in our own code, but also in a consumer's code. That makes this a change for v4, if it's a change we want to make.

This change in isolation would not have an effect on consumers. They'd still be able to do things like use color() without any explicit imports because sewing-kit would continue to add the foundation and shared files as a prefix to sass files as per https://github.com/Shopify/sewing-kit/blob/9d67951f995e293f58c59415aa1c8f1f37376995/packages/sewing-kit/src/tools/webpack/config/utilities.ts#L56-L59. We should eventually move to stopping implicit imports in consuming projects too, but we can make that decision and breaking change separate to this change (think of this proposal as the first non-breaking step to enable that future breaking change).

Strong agree on this being an opportunity to rethink our sass api and shrinking what we expose to consumers - things in the foundation folder feel somewhat reasonable, but I don't think a lot of the stuff in shared should be used by external people. My gut feel is eventually I'd like to move those foundational pieces into polaris-tokens and make people import from tokens instead of polaris-react but that needs some deeper thought but I think we can move ahead with this proposal without making a decision on how to shrink our API surface area.

[tsov]

this would represent a serious breaking change

That would depend on the consumers app/webpack setup, merging this and not changing sewing kit will result in consumers still being able to magically use shared/foundational exports.
So this change would not block consumers from updating/upgrading polaris if that is the concern.

We'd probably to ensure this stay like this somehow because even 1 selector can add up.

We may be able to write a test that checks if any file inside foundation/**/*.scss or shared/**/*.scss exports a selector/css rule?

[danrosenthal]

Thanks for the clarification Max and Ben. Given that, I’m comfortable with this change.

Ben, I’m excited to hear you’re already thinking about our SASS APIs. Tokenizing that sort of stuff makes a lot of sense.

I’d love it if SASS was considered an implementation detail that was entirely obfuscated from the consumer. It feels like a leaky abstraction today.

[kaelig]

I’d love it if SASS was considered an implementation detail that was entirely obfuscated from the consumer. It feels like a leaky abstraction today

Can you say more about this?

[BPScott]

I'd be interested to look at moving towards a world where polaris-react has no Sass API that can be used by consuming apps (e.g. people doing color('red') etc). Instead we should push consumers towards importing those things from polaris-tokens. Whether consumers would have to switch to using the token variables like $color-red or if we ship the color() function as part of tokens is up for grabs. The former is good for minimizing our surface area and having one way to do things, the later would result in minimal disruption as consuming apps already use color() they just get it from us atm.

[danrosenthal]

I’d love it if SASS was considered an implementation detail that was entirely obfuscated from the consumer. It feels like a leaky abstraction today

Can you say more about this?

So we create an abstraction on top of styles, markup, and scripts in the form of a component. A good abstraction should conceal the complexity of implementation from the consumer, and not force them to understand the underlying principles. Our use of SASS is an implementation detail in how we build components. In Polaris' current state, consumers' need to understand SASS, and our underlying SASS architecture, in order to extend (or in some cases, use) the system in their apps. That is a leaky abstraction.

The need to expose these SASS functions/mixins/variables might also reveal a shortcoming of the system, because, if it's necessary, that means we are not offering the right component primitives to assemble the patterns our consumers need.

If an idea is so complex that it can only be accomplished using a mixin or SASS function, it should be delivered in the form of a component primitive. Less complex units, like shared breakpoints, colors, spacing, and typography should be shared as design tokens instead of SASS. This means that a consumer no longer needs to be tied to our implementation details or understand them, and is instead free to implement their consuming app and extend our component offerings however they want.

[danrosenthal]

I'd be interested to look at moving towards a world where polaris-react has no Sass API that can be used by consuming apps (e.g. people doing color('red') etc).

I couldn't agree more. And if consuming apps like Shopify/web need this, they could simply copy our SASS into their project and consume it there.

Instead we should push consumers towards importing those things from polaris-tokens. Whether consumers would have to switch to using the token variables like $color-red or if we ship the color() function as part of tokens is up for grabs. The former is good for minimizing our surface area and having one way to do things, the later would result in minimal disruption as consuming apps already use color() they just get it from us atm.

I'd argue that we should kill color() entirely (both for consumers and internally), and only use tokens across the board. I feel the same about our spacing() function. I would also be happy to get rid of our media query mixins as well, and go back to using plain old media queries with values pulled from tokens. To me, these all represent redundancies, and an unnecessary maintenance burden.

[tmlayton]

TL;DR I’m good with this and don’t find the explicit imports contentious, rather net neutral. There is value in the explicitness.

no Sass API

And yes, no Sass API 💯.

[beefchimi]

Does this mean we are going to double our efforts on polaris-tokens 😛 I would LOVE to see more properties tokenized!

[danrosenthal]

Any in particular @beefchimi?

[ry5n]

I love the explicit Sass includes.

On the topic of removing the Sass API, as long as there’s a good styling system for styling new components, we’re good. If that can be done with tokens, that’s great. (I think people will always need to make new components.)

We should be careful we’re not missing use cases in the process though. For example, we might want something that can take tokens and generate accessible content colors to fill out an apps’s specific color palette. A public Sass API would be a good tool for that job.

[BPScott]

Updated this to use a single named import and it's looking a lot less intimidating.

I also updated our partial files to prefix them with an underscore per sassy conventions. This means that you can cmd+click on the import lines and they'll take you to the relevant file (well, as long as there's no hyphen in the filename thanks to silly VSCode bugs, but I've got a PR for that)

This isn't ready to go just yet, as the built output of the styles folder doesn't work due to changing relative paths. Once #1764 is merged that problem gets neatly sidestepped though so we're waiting on that PR.

[BPScott]

This is now ready for review 🎉

This introduces a new src/styles/_common.scss that all our scss files import.

We settled on having a single import as it's a bit less boilerplatey than having two. This in combination with distributing compiled builds in #1764 means that we never use shared.scss or foundation.scss in our code anymore - they're only used by projects that use sewing-kit and others consuming our sass build. This means we can add new variables/mixins in the styles folder, and as long as we add them to _common.scss but not shared.scss/foundations.scss we don't expose them to the world.

To Test

• Run a build on master, and make a copy of styles.css and the styles folder.
• Run a build on this branch. Compare the output of styles.css, which should be identical, and ensure there are no changes beyond path changes in shared.scss and foundation.scss in the styles folder

[tmlayton]

I do not have time to review the code, but I approve the idea 👍

[amrocha]

I think I would prefer to be even more explicit about imports and import each file individually, but I can see why people liked this approach better.

Definitely try this out in a big consuming project like the styleguide before shipping though

[amrocha]

Why do you need these?

[BPScott]

Stylelint doesn't like it if you're in a partial file (i.e one starting with an _) and you then import other files.

I assume because of sass's clunky import system being fine with you importing the same file multiple times they want you to put all your imports in a single top-level file rather than depending on stuff on a per-file basis to make it clear you're not repeating yourself. If you put imports within imports it won't be clear if you accidentally import the same file twice.

We're choosing to say we know better and that in this constraint case we're fine as only this spacing file will import the spacing tokens (and the color file will import the color tokens etc).

[amrocha]

I thought files prefixed with _ weren’t included by default, is that not the case?

[BPScott]

Nope all scss files get copied over. I reworked our sass build in #1764 but even before then we always included files starting with an underscore.

[BPScott]

I think I would prefer to be even more explicit about imports and import each file individually, but I can see why people liked this approach better.

I tried that originally but trawling the dependency graph of "this shared file depends on this foundation file" got complicated and ugly in our component files and it was hard to see if you forgot an import (sass doesn't complain if you call a nonexistent function because it might be valid css). Having a single import is quite a bit easier to reason about - "just import this and you're done".

There's slightly more overhead as you're defining functions/mixins you don't need but it doesn't make a noticeable difference in build time