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

Clean up code base #19

Open
aparlato opened this issue Apr 18, 2023 · 1 comment · May be fixed by #21
Open

Clean up code base #19

aparlato opened this issue Apr 18, 2023 · 1 comment · May be fixed by #21
Assignees

Comments

@aparlato
Copy link
Collaborator

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.

This was referenced Apr 18, 2023
@aparlato aparlato self-assigned this Apr 19, 2023
@aparlato
Copy link
Collaborator Author

aparlato commented Apr 19, 2023

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.

@aparlato aparlato linked a pull request Apr 28, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant