Variable overrides in MSS

[whitelynx]

Variables which are set multiple times do not behave as they do in Less.

Expected

From http://lesscss.org/features/#variables-feature-lazy-loading:

When defining a variable twice, the last definition of the variable is used, searching from the current scope upwards. This is similar to css itself where the last property inside a definition is used to determine the value.

Observed

Once set, a variable's value does not change.

This causes a couple of secondary problems:

• Emulating default values for variables does not work as expected.
  Since MSS doesn't support @import, we can't even work around this using something like:

@someVar: #fff;
@import "anotherFile.mss";
.someRuleOverride {
    polygon-fill: #f0f;
}

• Calling render multiple times on the same carto.Renderer instance does not work as expected; if any variables are used in the MSS of both render calls but set to different values in each, the second call's values will be ignored, instead using values from the first call.

Tests

I have added two new rendering tests in https://github.com/whitelynx/carto/tree/bug-var-overrides:

• rendering/var_override_simple: loads a single MSS file which sets the same variable twice (we should see the second value used, but instead the first is used)
• rendering/var_override: loads two MSS files; the first is from the rendering/var_concat test, and sets a variable; the second is a new MSS file that just sets that variable to a new value (we should see the value from the new MSS file being used, but instead the one from the rendering/var_concat test is used)

[tmcw]

Probably here, in which we choose the first instance rather than the last https://github.com/mapbox/carto/blob/master/lib/carto/tree/variable.js#L25 (note that less compatibility is not a goal of carto, and a behavioral change like this could cause more fallout than win)

[Morgul]

It might be worth expounding on the use case. Let's say you're writing a TMS or WMS service that under the hood uses Carto/Mapnik. Now, let's say, you want users to be able to control the colors for some of the things you're drawing (for instances, a farmer's fields). By default, you draw all corn fields yellow, but you've got a user who would rather draw them blue. There's very little technical reason I can see not to allow that. (And in our particular use case, a feature like this is a big selling point for our company.)

Currently, the only options we've found are to include the .mss files in reverse order (which can have other side effects), or to eschew the use of variables entirely, and use some sort of template engine in place of variables.

With the exception of some single-assignment languages, I don't know of any instance where the desired behavior would be surprising; re-assigned variables always override previous values. That's how it works in Javascript, Python, C/C++, etc. Even CSS rules work this way. At the very least, this behavior needs to be documented, as it's a deviation both from expectations, and from less (which is the only place users have to go when Carto's docs don't explicitly answer questions).

I do understand the desire to limit/prevent breakages. I'd be curious if there was any way to judge how large a break this might cause for people... I suspect the only instances of breakage would be accidental reliance on this behavior rather than explicitly desiring this. Otoh, it seems like it might be hard to judge.

[springmeyer]

Thanks for the additional thoughts @Morgul. I agree that supporting this would be unlikely to cause breakages (though we'll need to be careful for sure). And while in no way does Carto promise less.js compatibility, following less in this respect seems reasonable. Do you or @whitelynx have the means to provide a patch for this?

[Morgul]

We do. We'll submit a PR for this shortly (if not today, then monday.)

[springmeyer]

How is the work on this going? I'm planning a new TileMill release in the coming weeks and would be nice to see this land.

[springmeyer]

@Morgul and @whitelynx - bump.

[Morgul]

We ended up working around the issue and getting pulled off onto other things at work. That being said, @whitelynx should be able to take a bit of time in the next week to get this worked out.