-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Proposal for configuring modules through imports #2773
Merged
Merged
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
f505a4c
Initial draft of proposal
jathak aeefdc1
Make sure implicit configs work with prefixed forwards
jathak b5a8157
Expand on design decisions
jathak 3e9d982
Updates based on feedback
jathak be61582
Refactor long sentence in Background
jathak 7fb4d49
Give the proposal a short name
nex3 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,225 @@ | ||
# Configuring Modules Through Imports: Draft 1 | ||
|
||
*([Issue](https://github.com/sass/sass/issues/2772))* | ||
|
||
This proposal modifies the module system semantics to support configuring | ||
libraries that have migrated to the module system through `@import` rules in | ||
downstream stylesheets without requiring changes to those stylesheets. | ||
|
||
## Table of Contents | ||
|
||
* [Background](#background) | ||
* [Summary](#summary) | ||
* [Design Decisions](#design-decisions) | ||
* [Definitions](#definitions) | ||
* [Procedures](#procedures) | ||
* [Semantics](#semantics) | ||
* [Executing Files](#executing-files) | ||
* [Importing Files](#importing-files) | ||
|
||
## Background | ||
|
||
> This section is non-normative. | ||
|
||
As it is, while configuration in a `@use` rule passes through `@forward` rules | ||
automatically, there's no way for a stylesheet using `@import` to configure | ||
members that are behind a `@forward` rule. | ||
|
||
This makes it difficult for libraries with configurable variables to migrate to | ||
the module system without breaking downstream users that haven't migrated yet, | ||
especially if the library removed a manual prefix from its members during | ||
migration, as it would usually preserve the original names for downstream users | ||
by creating an import-only file that forwards the regular stylesheet with a | ||
prefix. | ||
|
||
## Summary | ||
|
||
> This section is non-normative. | ||
|
||
This proposal modifies the semantics for configuring a module when `@import` is | ||
involved to ensure that most downstream users of a library are not broken when | ||
the library migrates to the module system. | ||
|
||
When a file is loaded by an `@import` rule, a [configuration][] is created that | ||
includes all variables declared in the current [import context][]. This | ||
implicitly created configuration is a special type that can be distinguished | ||
from other, explicitly created configurations. | ||
|
||
When a `@forward` rule is encountered within a file that was loaded by an | ||
`@import` rule, the implicit configuration is passed to it in the same way as an | ||
explicit configuration from a `@use` rule would be. | ||
|
||
Normally, when a module has already been executed, and is then loaded with a | ||
configuration that is not empty, an error is thrown. However, if the | ||
configuration is an implicit one, this error will be ignored and the executed | ||
module will be returned in the same way as if the configuration were empty. If | ||
an implicit configuration passes through a `@forward` rule with a prefix, then | ||
new configuration created for that rule is also considered an implicit one and | ||
retains this special property. | ||
|
||
This proposal should allow most existing stylesheets using `@import` to continue | ||
working unchanged after a library they depend on migrates to the module system. | ||
|
||
[configuration]: ../accepted/module-system.md#configuration | ||
[import context]: ../accepted/module-system.md#import-context | ||
|
||
### Design Decisions | ||
|
||
We considered a few alternatives in designing this proposal. | ||
|
||
One alternative did not involve any language changes at all, instead | ||
recommending that library authors add `@use` rules explicitly configuring their | ||
variables to their [import-only files][] when migrating. For example: | ||
|
||
```scss | ||
// app.scss | ||
$lib-color: blue; | ||
@import "library"; | ||
|
||
// _library.scss | ||
$color: green !default; | ||
|
||
// _library.import.scss | ||
@use "sass:meta"; | ||
@use "library" with ( | ||
$color: if(meta.variable-defined("lib-color"), $lib-color, null) | ||
); | ||
@forward "library" as lib-*; | ||
``` | ||
|
||
While this would work for simple libraries with a single entrypoint, libraries | ||
with multiple components that depend on common sublibraries but can be imported | ||
separately would often break, as this solution would attempt to configure some | ||
modules more than once. The same would happen if you imported even a simple | ||
library more than once. | ||
|
||
An alternative to just ignoring subsequent implicit configurations would be to | ||
(a) filter them to include only variables that are actually configurable and | ||
(b) allow the subsequent configuration only if it exactly matched the previous | ||
one, but doing this matching could hurt performance. This would also still cause | ||
issues if the same library is imported more than once. | ||
|
||
While the solution we settled on does not perfectly cover all use cases that | ||
worked before the library migrated to the module system, we think it strikes a | ||
good balance of supporting most existing use cases without hurting performance | ||
or making the language specification and implementation overly complicated. | ||
|
||
One potential use case that will still not work is if a downstream user of the | ||
library attempts to change its configuration between two imports of the same | ||
library, that change will be ignored. However, this is an edge case that is (a) | ||
probably not intended by the user, (b) relatively easy to fix by moving all | ||
declared configuration variables before all library imports, and (c) very | ||
difficult to support for a library using the module system without compromising | ||
the module system's [goals][]. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest being more specific about how it compromises which goals. |
||
|
||
[import-only files]: ../accepted/module-system.md#import-compatibility | ||
[goals]: ../accepted/module-system.md#goals | ||
|
||
## Definitions | ||
|
||
This proposal modifies the definition of a [configuration][] within the | ||
[module system spec][] to add the following: | ||
|
||
A configuration is either *explicit* or *implicit*. When a configuration is | ||
created, if the type is not specified, it is considered *explicit*. | ||
|
||
[module system spec]: ../accepted/module-system.md | ||
|
||
## Procedures | ||
|
||
This proposal modifies the fourth bullet of the [Loading Modules][] procedure | ||
within the [module system spec][] to read as follows: | ||
|
||
|
||
* If `file` has already been [executed][]: | ||
|
||
* If `config` is **explicit and** not empty, throw an error. | ||
|
||
* Otherwise, return the module that execution produced. | ||
|
||
[Loading Modules]: ../accepted/module-system.md#loading-modules | ||
[executed]: ../accepted/module-system.md#executing-files | ||
|
||
## Semantics | ||
|
||
### Executing Files | ||
|
||
This proposal modifies the first bullet of the semantics of [Executing Files][] | ||
within the [module system spec][] to read as follows: | ||
|
||
* If this file isn't being executed for a `@forward` **or `@import`** rule: | ||
|
||
* For every variable name `name` in `config`: | ||
|
||
* If neither `file` nor any source file for a module transitively forwarded | ||
or imported by `file` contains a variable declaration named `name` with a | ||
`!default` flag at the root of the stylesheet, throw an error. | ||
|
||
This proposal also modifies the fifth bullet to read as follows: | ||
|
||
* When a `@forward` rule `rule` is encountered: | ||
|
||
* If `rule` has an `AsClause` with identifier `prefix`: | ||
|
||
* Let `rule-config` be an empty configuration. **`rule-config` is implicit | ||
if `config` is implicit and explicit otherwise.** | ||
|
||
* For each variable `variable` in `config`: | ||
|
||
* If `variable`'s name begins with `prefix`: | ||
|
||
* Let `suffix` be the portion of `variable`'s name after `prefix`. | ||
|
||
* Add a variable to `rule-config` with the name `suffix` and with the | ||
same value as `variable`. | ||
|
||
* Otherwise, let `rule-config` be `config`. | ||
|
||
* Let `forwarded` be the result of [loading][] the module with `rule`'s URL | ||
and `rule-config`. | ||
|
||
* [Forward `forwarded`][] with `file` through `module`. | ||
|
||
[Executing Files]: ../accepted/module-system.md#executing-files | ||
[loading]: ../accepted/module-system.md#loading-modules | ||
[Forward `forwarded`]: ../accepted/module-system.md#forwarding-modules | ||
|
||
### Importing Files | ||
|
||
This proposal modifies the semantics for [Importing Files][] within the | ||
[module system spec][] to read as follows: | ||
|
||
This algorithm takes a [source file][] `file`, an [import context][] `import`, | ||
and a mutable [module][] `module`. | ||
|
||
* If `file` is currently being executed, throw an error. | ||
|
||
* **Let `config` be an implicit configuration containing every variable defined | ||
in `import`.** | ||
|
||
> If `file` does not contain any `@forward` rules, `config` will never be | ||
> used, so implementations may wish to skip this step and use the empty | ||
> configuration instead in that case for performance reasons. | ||
|
||
* Let `imported` be the result of [executing][] `file` with ~~the empty | ||
configuration~~ **`config` as its configuration** and `import` as | ||
its import context, except that if the `@import` rule is nested within | ||
at-rules and/or style rules, that context is preserved when executing `file`. | ||
|
||
* Let `css` be the result of [resolving extensions][] for | ||
`imported`, except that if the `@import` rule is nested within at-rules and/or | ||
style rules, that context is added to CSS that comes from modules loaded by | ||
`imported`. | ||
|
||
* Add `css` to `module`'s CSS. | ||
|
||
* Add `imported`'s [extensions][] to `module`. | ||
|
||
* Add each member in `imported` to `import` and `module`. | ||
|
||
[Importing Files]: ../accepted/module-system.md#importing-files | ||
[source file]: ../accepted/module-system.md#source-file | ||
[module]: ../accepted/module-system.md#module | ||
[executing]: ../accepted/module-system.md#executing-files | ||
[resolving extensions]: ../accepted/module-system.md#resolving-extensions | ||
[extensions]: ../accepted/module-system.md#extension |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This first sentence is kind of awkward. Consider breaking it up into two sentences instead.