Default SCSS variables prevent use of multiple fonts

[davidjb]

Describe the bug

The introduction of #129 (default variables for SCSS) has caused problems when trying to include multiple fontsource fonts in a build. Because variables are global, the first-included font SCSS sets variables which thus break any others down the line, affecting font names, paths, unicode maps etc. Additionally, because variables are very generically named ($style, $display etc), this makes integrating fonts that much harder into a project.

To Reproduce

@import "@fontsource/open-sans/scss/mixins";
@include fontFace();

@import "@fontsource/playfair-display/scss/mixins";
// This runs the playfair-display fontFace mixin, but all its variables
// and output refer to what was defined in open-sans
@include fontFace();

Expected behavior Multiple fonts can be included and not affect others.

The defaults could be moved onto the @mixin fontFace definition, meaning defaults are scoped and thus won't affect the global scope. The result is you could do something like this:

$my-font-weight: 500;
@import "@fontsource/open-sans/scss/mixins";
@include fontFace($weight: $my-font-weight);
@import "@fontsource/playfair-display/scss/mixins";
@include fontFace($weight: $my-font-weight);
@include fontFace($weight: 300);

This is my preference as it avoids the global scope entirely, requiring variables to set explicitly when mixing in -- doing this makes things clearer for the reader.

An alternate option could be to namespace the variables (e.g. $openSansFontName etc) so if someone wanted multiple fonts to all share the same weight etc, then they could do it like so:

$my-font-weight: 500;
$openSansFontWeight: $my-font-weight;
$playfairDisplayFontWeight: $my-font-weight;
@import "@fontsource/open-sans/scss/mixins";
@include fontFace();
@import "@fontsource/playfair-display/scss/mixins";
@include fontFace();

However, this burdens the implementer / reader to understand the side effects of setting a given global variable because the link between the variables and fontFace() isn't immediately apparent.

Screenshots N/A

Additional context Add any other context about the problem here.

[ayuhito]

The defaults could be moved onto the @mixin fontFace definition, meaning defaults are scoped and thus won't affect the global scope. The result is you could do something like this:

Are they not already? The issue is using @import makes those variables global in the first place. I have a partial solution for that in mind as I could just remove the !default flag for variables like $fontName while leaving variables that usually get changed globally such as $display with the default flag. That way, necessary variables do not overwrite each other unless the developer changes the variable AFTER the @include is called. Once a new @include is called, the variables without the default flag will reset any changes made which is a janky solution to the above error.

Now that I have looked more into #130, this should fix the scoping issue entirely and their values won't overlap. However, the example given won't work:

@use "@fontsource/open-sans/scss/mixins" as OpenSans;
@use "@fontsource/barlow-condensed/scss/mixins" as Barlow;

$fontDir: "/dist/node_modules/@fontsource/"
$display: optional;

@include OpenSans.fontFace($weight: 400, $style: normal);
@include Barlow.fontFace($weight: 400, $style: normal);

The variables $display and $fontDir will not apply to the @includes. I need to research into this more and see if there are other Sass functions I could use to look into finding a solution for that. I don't have much time these days though.

@cbirdsong, do you have any ideas or thoughts toward the matter? Given you were involved with helping us flesh out the idea for default variables.

[lonix1]

Yes just ran into this too! :smile:

I also like the local scoping idea.

I read the scoping docs but didn't find anything that seemed obviously useful.

[ayuhito]

I'm going to post a quick hotfix that removes the !default flag for all variables within the next couple hours (because this essentially BREAKS every setup which imports more than one font), but I do want to find a more appropriate and stable solution for this. It's essentially a partial revert.

It isn't the prettiest solution, but far better than what's live now.

[cbirdsong]

I have a partial solution for that in mind as I could just remove the !default flag for variables like $fontName while leaving variables that usually get changed globally such as $display with the default flag.

I’m thinking this is the best path right now.$display is an easy one to include, and $fontDir would also be one if it was just the root and did not include $fontid. I can’t really judge on the unicode range-related ones, but it seems reasonable that you’d want to set something relating to character sets globally.

I’m also not sure my example is up to date with the new @use-based Sass module system. This example from the docs shows a different way to override default variables:

@use 'library' with (
  $black: #222,
  $border-radius: 0.1rem
);

Either way that doesn’t address backwards compatibility with older Sass versions.

[thasmo]

It isn't the prettiest solution, but far better than what's live now.

Has this issue actually been fixed for usage via @import?

[ayuhito]

Has this issue actually been fixed for usage via @import?

Not fully nor cleanly. Arguably, it was a partial rollback of #129 so you have half a feature and half of something broken 😅

[jwr1]

Anyone interested in the new Sass integration can see the mixins.scss file here. I have also written documentation for the two new mixins, faces and generator. Nothing is set in stone until fontsource v5 is released, so if you find any bugs or have feedback on the documentation, or the mixins themselves, please share :+1:.

I also want to thank @thasmo for getting us started in https://github.com/fontsource/fontsource/discussions/384 :tada:.

[jwr1]

We can still change the mixin and parameter names too, in case someone has a better name for faces, generator, or any of the parameters.

[ayuhito]

We've rewritten the Sass integration entirely for v5 🎉 Please let us know if you run into any issues!