Skip to content
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

Support configuring modules through imports #885

Merged
merged 7 commits into from Nov 26, 2019

Conversation

@jathak
Copy link
Member

jathak commented Nov 19, 2019

@jathak jathak force-pushed the import-configuration branch 3 times, most recently from b22f6b9 to 4d00823 Nov 19, 2019
Copy link
Contributor

nex3 left a comment

What do you think about, instead of producing a Configuration from an Environment, defining a subclass of Configuration that wraps an Environment? That way you have to do as little work and allocation as possible when you create the Configuration, and you mostly just pay the price if there are actual default variables that might be overridden.

@jathak

This comment has been minimized.

Copy link
Member Author

jathak commented Nov 19, 2019

I think that makes sense, but could there ever be a case where the Environment is mutated after the Configuration is created but before it's used?

@nex3

This comment has been minimized.

Copy link
Contributor

nex3 commented Nov 19, 2019

Good question... I guess that does run into the problem where variables defined in the midstream file suddenly become part of the upstream file's configuration, which we don't want, so copying everything over is probably better in the end.

_variableNodes == null ? <String, AstNode>{} : _variableNodes[i];
for (var name in values.keys) {
configuration[name] =
ConfiguredValue(values[name], nodes[name]?.span, nodes[name]);

This comment has been minimized.

Copy link
@nex3

nex3 Nov 19, 2019

Contributor

Materializing AstNodes' spans can be expensive, which is why we pass them around as AstNodes in the first place, so I'd like to avoid nodes[name]?.span here. In practice this span is also never used, since an implicit configuration's variables are never invalid.

We could just pass null here, or we could encode this more statically by having a subclass with a constructor ImplicitConfiguredValue(Value, AstNode) and defining Configuration.implicit(Map<String, ImplicitConfiguredValue>). Up to you.

/// A map from variable names (without `$`) to values.
///
/// When this is empty, it may be unmodifiable, so [Map.remove] should not be
/// called on this.

This comment has been minimized.

Copy link
@nex3

nex3 Nov 19, 2019

Contributor

Now that we have a full Configuration class, we can enforce this more aggressively by only exposing an unmodifiable view of this map and exposing a Configuration.remove() method that handles empty configurations correctly.

final Map<String, ConfiguredValue> values;

/// Whether or not this configuration is implicit.
final bool isImplicit;

This comment has been minimized.

Copy link
@nex3

nex3 Nov 19, 2019

Contributor

I might move the discussion of what an implicit configuration means from the class documentation to this getter.

isImplicit = false;

bool get isEmpty => values.isEmpty;
bool get isNotEmpty => values.isNotEmpty;

This comment has been minimized.

Copy link
@nex3

nex3 Nov 19, 2019

Contributor

Typically classes that aren't implementing core library interfaces don't define isNotEmpty. (Defining isNotEmpty at all was a design mistake—having a different way to negate exactly one boolean getter doesn't make sense—but there's no going back on that for the core libraries at this point.)

bool get isNotEmpty => values.isNotEmpty;

/// Creates a new configuration from this one based on a `@forward` rule.
Configuration throughForward(ForwardRule forward) {

This comment has been minimized.

Copy link
@nex3

nex3 Nov 19, 2019

Contributor

Moving this logic here is nice.

: Configuration({...values}, isImplicit: isImplicit);
}

/// A variable value that's been configured using `@use ... with`.

This comment has been minimized.

Copy link
@nex3

nex3 Nov 19, 2019

Contributor

Some values are configured without @use ... with—maybe just say "configured for a Configuration"?

var url = stylesheet.span.sourceUrl;

var alreadyLoaded = _modules[url];
if (alreadyLoaded != null) {
if ((configuration ?? _configuration).isNotEmpty) {
configuration ??= _configuration;

This comment has been minimized.

Copy link
@nex3

nex3 Nov 19, 2019

Contributor

I don't think you want to make this assignment. We use the fact that this was passed as null later on to determine whether to throw errors about unused configuration variables.

@@ -635,8 +632,7 @@ class _EvaluateVisitor
_inKeyframes = false;

if (configuration != null) {
_configuration =
configuration.isEmpty ? const {} : Map.of(configuration);
_configuration = configuration.clone();

This comment has been minimized.

Copy link
@nex3

nex3 Nov 19, 2019

Contributor

Style nit: you can make this a single-line if now.

@@ -661,11 +657,11 @@ class _EvaluateVisitor
if (configuration != null && _configuration.isNotEmpty) {

This comment has been minimized.

Copy link
@nex3

nex3 Nov 19, 2019

Contributor

Shouldn't this also check whether the configuration is implicit?

_importer = importer;
_stylesheet = stylesheet;
_root = ModifiableCssStylesheet(stylesheet.span);
_parent = _root;
_endOfImports = 0;
_outOfOrderImports = null;

// This configuration is only used if it passes through a `@forward`
// rule, so we avoid creating unnecessary ones for performance reasons.
if (stylesheet.forwards.isNotEmpty) {

This comment has been minimized.

Copy link
@nex3

nex3 Nov 19, 2019

Contributor

Before you submit, try removing this condition and always creating the environment, and run the result against sass-spec. It shouldn't change the behavior, so if it does you'll have found a bug.

jathak added 3 commits Nov 19, 2019
@jathak jathak force-pushed the import-configuration branch from 4d00823 to 26bb474 Nov 25, 2019
jathak added 2 commits Nov 25, 2019
@jathak jathak requested a review from nex3 Nov 25, 2019
@jathak

This comment has been minimized.

Copy link
Member Author

jathak commented Nov 25, 2019

Okay, I think I've addressed all comments. I also tested with the condition for creating the implicit configuration removed and confirmed that all of the specs still pass.

@nex3
nex3 approved these changes Nov 26, 2019
Copy link
Contributor

nex3 left a comment

Very nice work!


/// The [AstNode] where the variable's value originated.
///
/// This is used to generate source maps.

This comment has been minimized.

Copy link
@nex3

nex3 Nov 26, 2019

Contributor

Maybe mention that this can be null if source map generation is disabled.

@jathak jathak merged commit 8270dc1 into master Nov 26, 2019
3 checks passed
3 checks passed
cla/google All necessary CLAs are signed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jathak jathak deleted the import-configuration branch Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.