stamen / mapbox-gl-style-diff

Other
2 stars 1 forks source link

Clean up code base #19

Open aparlato opened 1 year ago

aparlato commented 1 year ago

As mentioned in https://github.com/stamen/mapbox-gl-style-diff/pull/18#issue-1671405790 and followup from an early PR: https://github.com/stamen/mapbox-gl-style-diff/pull/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.

aparlato commented 1 year ago

Specifically we could take one of two approaches:

1) Use diffing directly from the style spec

The code in diff.js is copypasta'd from Mapbox GL's style spec and given additional custom functionality and formatting.

However, that function is already exported from the style spec.

Instead of our edited version of this file, it would be much cleaner and more legible (not to mention easier to stay up to date spec-wise) if we used the exported function and only wrote code in this repo that makes that output more usable and is clear about the structure of that response before (from the differ) and after.

import { diff } from '@mapbox/mapbox-gl-style-spec';

const formatOutput = (output) => {
  ...
}

const diffStyles = (a,b) => {
  ... add some definitions of what this output looks like
  let output = diff(a,b);

  return formatOutput(output);
}

2) Write the diff code from scratch

The entire structure of the diff.js file is around determining what Mapbox GL functions/commands should be run (eg setStyle). This is confusing for our purposes since we aren't running those commands and really we're treating this like a fancy JSON diff.

We could write this (either based on diff.js or not) much more simply since at the end of the day, we really just need to go through a stylesheet and for each relevant section, determine if the before is different than the after and log that out as before: ..., after: .... Any further diffing is already handled on the consumer (eg determining which parts of an expression specifically changed.

Mapbox GL style recurse could be a helpful tool for us with this approach to check style properties. Layer additions/removals, and root level changes are already easy for us to diff without much code.

Moving forward

I don't have a strong opinion on which approach we take, but I probably lean towards 2 just because it reflects the level of simplicity/complexity actually involved in what this repo does. It may depend a little on the future of this repo and what else we might use this for.