stamen / mapbox-gl-style-diff Goto Github PK

Per #10 (comment) let's move the standalone diff viewer into its own repo: mapbox-gl-style-diff-viewer.

This will mean we can remove the copy-to-S3 Github action in this repo as well. Once the new repo is created, we can figure out the best way to continue copying the viewer into S3.

cc @ebrelsford

Per this comment, I think this repo would benefit from unit tests. Since the diffing is intended as a QA step, we want to make sure a break in the code doesn't result in not showing important differences to the user.

@peterqliu what do you think of breaking down diff.js into different files (or at least different exported functions) (eg diffLayerPropertyChanges) and testing each individually with test cases that have the known changes we want to diff for?

I suggest using Jest over in https://github.com/stamen/amazon-MapStyleSandbox/issues/320 since I'm familiar with it, but open to whatever.

#18 (comment)

Updating sources appears to give a less helpful diff than it could by showing a changed source as a removal + an addition.

We should reconsider how we handle this as it's a common use case with our clients to update the source URL of tiles. This likely will/should happen as part of #19

Changes in layers and layer properties are supported. Let's add deltas for adding/changing/removing sources.

We currently detect any and all changes in each layer property. For very long property expressions, it can be difficult to visualize a small change in a large object or array.

Instead of the usual "new value old value" format, we can iterate through the expression, pinpoint the parts that changed, and visualize accordingly ("new old value").

Per shareshare, @almccon mentioned that some style changes (e.g. applying or undoing a default style value) don't result in a visual change, and we can flag that.

This is doable for most properties, by comparing values against a list of defaults in the style spec. As a first iteration we can just compare literal values, though expressions can also evaluate to equivalent values.

Per previous conversations, we should turn this repo into a module. That way we can use the code here in a standalone diff tool like this repo currently contains and replace this copy-pasta file in Maputnik that allows us to use diffing there. It would also allow us to use the output in a Github action if we wanted this to run on opening PRs.

The copied code in Maputnik uses an additional function to transform the current diff output into something more readable for our purposes. This output format is something we should consider using directly from this module since the current output reflects a diff from a setStyle in GLJS.

As mentioned in #18 (comment) and followup from an early PR: #10

Also, the format from diff.js gets updated by an additional function appended to diff.js for the sake of simplicity/keeping this work scoped small (that function is now named diffStyles and the previous output comes from diffStylesSetStyle), but ideally we'd remove this additional formatting function and just update the rest of the diffing function code to output to this format without further alteration.

This code feels pretty crufty since the original code was borrowed for Mapbox GL JS code to set properties and then we appended it for our purposes. This mostly works, but makes further development difficult since the code is not at all clean.

Ideally we can clean this code up to be more understandable and (probably as follow up) test it.

Describe the issue

I'm using the differ to investigate changes made in this PR (https://github.com/stamen/amazon-MapStyleSandbox/pull/468), but am seeing different results between Chrome and Firefox. Firefox is showing the results I expect, whereas it seems like Chrome is using an outdated version of the stylesheet from main (despite the URL showing the correct path).

Differ URL: https://stamen-styles.s3.us-west-2.amazonaws.com/dist/master/stamen-diff.html?#lstyle=https://stamen-styles.s3.us-west-2.amazonaws.com/dist/main/variants/gaia_en/style.json&rstyle=https://stamen-styles.s3.us-west-2.amazonaws.com/dist/align-zooms/variants/gaia_en/style.json

output in Chrome (unexpected):

output in Firefox (expected):

Additional context

I'm seeing something similar in the viewer between Chrome and Firefox - could there be caching in one browser that is not reflected in another?

Currently, we use layer id's as the immutable property to keep track of layers between before and after. This means the differ can't make sense of any changes to that property (i.e. renaming layers).

There are clever things we can do to guess whether a new layer is probably actually a renamed one (the new layer having many of the same properties as an old, deleted one), but it's not fully reliable.

One way to track changes while supporting variable layer names is attaching a unique identifier to the layer metadata -- assigning it upon layer creation in Maputnik, and preserving that across all layer mutations until the layer is deleted.

Currently, the differ takes in rstyle and lstyle to fetch style url's. Aside from that and editing the raw JSON in the textareas, there's no other way to switch styles.

We should add a dropdown or url text input to make this easier.

This repo uses master as its primary branch. Let's go ahead and switch over to main.

@ebrelsford this is definitely low priority, but I think you may have to take this on due to permissions since I can't see branch settings.