Mxchaeltrxn recursive types 2

[Mxchaeltrxn]

@haydn

Based off #111

Note that I'm merging this into the branch you made just to make things (ever so slightly) easier for you to review. I know there's a lot here and I could have done a better job at splitting it up but hopefully it is okay because you have a good understanding of the components involved.

Okay so what I've done is:

1. Make resolvePath compatible with reconcilePath (as resolvePath was slightly broken after you added reconcilePath).
2. Moved resolvePath so that it gets called before AbstractRenderer is rendered. I think it's just simpler this way.
3. Implemented onChange using object-path-immutable.
4. Implemented onNavigate for lists and records.
5. Removed depth property.
6. Implemented path bar and back button (which sort of already there from before in case you forgot).

TODO:

• Implement checkbox renderers.
• Add proper styling.
• Is it worth refactoring AbstractRenderer so that conforms to the Open/close principle? It makes sense to in my head so changes don't have to continually be made to AbstractRenderer when adding a new renderer. It also reduces the total amount of code.

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/sketchbook-js/sketchbook/3biilu791
✅ Preview: https://sketchbook-git-mxchaeltrxn-recursivetypes-2.sketchbook-js.vercel.app

[haydn]

@Mxchaeltrxn I haven't done a full review of everything here yet, but I'm having a quick look through on my phone.

Is it worth refactoring AbstractRenderer so that conforms to the Open/close principle? It makes sense to in my head so changes don't have to continually be made to AbstractRenderer when adding a new renderer. It also reduces the total amount of code.

I don't quite follow you here. I think it's a bit confusing to apply the open/closed principle because we're using more of a functional programming paradigm rather than OOP, but that aside, I'd still say it conforms to being open to extension — that happens through composition. You could wrap this component in another to extend its functionality (the equivalent to extending a class in OOP). It's also closed to modification — without changing its source code there's no way for someone to change its behaviour.

However, I get what you're saying about needing to update it whenever we add a new renderer. No matter what, we're going to have to update something when a new renderer is added. Currently there are multiple places that needs to happen, which isn't ideal, but the setup we really want is for AbstractRenderer to be the only place we need to update. None of the other renderers should need to know about each other — they should just delegate to AbstractRenderer whenever they need to render another option. Currently this only applies for ListRenderer and RecordRenderer, but theoretically there could be other renderers in the future that are also recursive.

[haydn]

@Mxchaeltrxn Sorry, I also should've mentioned in my previous comment that the reason I was passing down depth was so that ListRenderer and RecordRenderer could decide how to render themselves using it (instead of the parent ListRenderer or RecordRenderer making that decision and requiring knowledge about the other renderer). If they're at a depth of more than 1, they can display themselves as a button to navigate deeper, but if they're up the top they can display as a full list. This is nice because the logic for each only exists in one place (even if we added another recursive renderer) and it lets us easily change the rules if we want to do something like render lists that are 1, 2 or 3 layers down, but show a button to navigate deeper for lists that are 4 or more layers down. Does that make sense?

[Mxchaeltrxn]

@haydn Oh that makes a lot of sense and I can see it reducing the overall amount of code in the long run. I'll fix that sometime this week.

Also, I made another pull request pointing to the old one because I was trying out stacked pull requests. I thought it might help you review 😅

[Mxchaeltrxn]

I decided to just do it since I understood the changes. I've also rebased from master.

It's always satisfying deleting code—a little less satisfying when it's your own code though. 😅

Thanks for the feedback! It makes a lot of sense.

[haydn]

@Mxchaeltrxn Fast work!

I've mocked-up some designs so we have a clearer idea about where we want to get to:

I reckon we can just do something really simple for the navigation (I've just replaced the heading with a back to go back up a level when we need it):

There's one pretty big problem this design introduces with the reordering of lists, but I'll save that for my next round of review. 😄

[haydn]

With the design in mind, I think there's problem with the prop that we're passing down here. We want to use the depth to choose how to render the options based on how deeply they're currently visually nested — not how far they're nested in the data — so I think all we're after here is:

Suggested change

	displayDepth={optionDepth}
	displayDepth={0}

[haydn]

I think there's a better way to handle these updates. Because we've got this nice recursive pattern happening, we can let the ListRenderer and RecordRenderer handle the complexity and none of the other components will need to do anything special (they wont need to provide their path when updating). I've put together a little demo of what I mean in CodeSandbox:

https://codesandbox.io/s/modern-darkness-gc6kc

In that example you can see that each component is passed an updateNode prop that is just an updater function. In the case where we recurse we create a new updater function for each child in created by wrapping the parent's updater function.

This will avoid the tricky problem you're using object-path-immutable to solve and instead we do something like this (I haven't actually tested this code, probably buggy):

Suggested change

	onChange={(newValue, rendererPath) => {
	// rendererPath is formatted like path1.path2.path3 etc.
	const formattedPath =
	`doc.layers.${selectedLayerIndex}.options.` +
	rendererPath.join(".");
	return setState(currState => {
	return immutable.set(currState, formattedPath, newValue);
	});
	}}
	onChange={(updater) => {
	setState((current) => ({
	...current,
	layers: current.layers.map((layer) => (
	layer.id === selectedLayer.id ? {
	...layer,
	options: updater(layer.options)
	} : layer
	))
	}));
	}}

[haydn]

Suggested change

	displayDepth={depth}
	displayDepth={depth + 1}

[haydn]

Suggested change

	displayDepth={depth}
	displayDepth={depth + 1}

[Mxchaeltrxn]

I just added some commits to handle the the onChange prop using recursion. The build fails because I haven't updated the tests yet.

Some questions for now:
I notice that I have logic in both resolvePath.js and also the renderers: RecordRenderer and ListRenderer to handle this behaviour. Initially, I thought it was not ideal but I don't really see a way around it. I was wondering if you did?

I'll do the rest of this a bit later!

[haydn]

@Mxchaeltrxn Nice!

Yeah, so… I'm actually thinking we do away with resolvePath. 😆 Hear me out!

We were using paths for 4 purposes:

1. Identifying options when updating values (onChange etc).
2. Handling navigation (keeping track of which option to show and identifying which option to navigate to when onNavigate is called).
3. Uniquely identifying elements using a key attribute in ListRenderer and RecordRenderer.
4. Rendering the breadcrumbs in the nav bar.

For point 3, we're going to run into real trouble down the line. If we make it possible for users to reorder items in a list (which we should), those paths aren't going to work because they use the index to identify the items in the list. When they get reordered, their path would change and React is going to get confused about which DOM element belongs to which item in state. This is explained in detail in the React docs.

This leaves us in a bit of a pickle. What we have to do is give each item in a list a unique ID, so instead of this:

const exampleLayer = {
  id: "123",
  type: "SketchbookComponent",
  component: "Example Component",
  name: "Layer 1",
  x1: 100,
  y1: 100,
  x2: 750,
  y2: 140,
  options: {
    scalarExample: true,
    listExample: [1, 2, 3],
    recordExample: {
      foo: "bar"
    }
  }
};

We'd need something like this:

const exampleLayer = {
  id: "123",
  type: "SketchbookComponent",
  component: "Example Component",
  name: "Layer 1",
  x1: 100,
  y1: 100,
  x2: 750,
  y2: 140,
  options: {
    scalarExample: true,
    listExample: [
      { id: "235", value: 1 },
      { id: "423", value: 2 },
      { id: "653", value: 3 }
    ],
    recordExample: {
      foo: "bar"
    }
  }
};

It's a bit gross, but if we're going to be forced down that path for values in lists, I think we may as well do the same thing for all the values:

const exampleLayer = {
  id: "123",
  type: "SketchbookComponent",
  component: "Example Component",
  name: "Layer 1",
  x1: 100,
  y1: 100,
  x2: 750,
  y2: 140,
  options: {
    id: "933",
    value: {
      scalarExample: { id: "345", value: true },
      listExample: {
        id: "325",
        value: [
          { id: "235", value: 1 },
          { id: "423", value: 2 },
          { id: "653", value: 3 }
        ]
      },
      recordExample: {
        id: "775",
        value: {
          foo: { id: "776", value: "bar" }
        }
      }
    }
  }
};

That may seem ridiculous, but it's got a big advantage — we can just use the ID's for all the things we're currently using paths for:

• onNavigate(id)
• <li key={id}>

That being said, I don't think we actually want to change the shape of what's persisted in the document (exampleDoc.js — that doc needs to be a bit human readable so that in the future developers are able to update them by hand if they make changes to a component that the document is using). These ID's are only needed temporarily when we've loaded the document into memory to work with it. So what we can do is create a little function that'll transform the persisted document format (the one without ID's) into the in memory document format (the one with ID's) at this point:

https://github.com/sketchbook-js/sketchbook/blob/master/src/Editor.js#L127

I've put together a CodeSandbox to demonstrate:

https://codesandbox.io/s/cl0m0

Probably worth mentioning that at some point in the future we'll probably use something like Recoil to maintain all this state. In some ways what we're doing here gets us a bit better aligned with that pattern.

I know there's a lot to take in there, if you'd like I can add a commit or two to point things in the right direction?

[Mxchaeltrxn]

@haydn That indeed is a pickle. That makes sense to me. It doesn't seem that gross if it's in memory. It'd be gross if it wasn't in memory though. 😁 I think I have enough to work with so you don't have to get me started (unless you'd like for this feature to be done more quickly). Thanks for the code sandbox and examples!

I'm assuming that since we don't have a path anymore, we will use something like breadth first search or depth first search to find the option to render?

[haydn]

I'm assuming that since we don't have a path anymore, we will use something like breadth first search or depth first search to find the option to render?

Yep, spot on. We can just search the tree to find the node. Even if that is expensive to do we can use useMemo to only recalculate it when the user navigates.

[Mxchaeltrxn]

@haydn Hey I’ve just written up a small plan/todo list since a few things have changed and new things have come up. Let me know how this sounds to you. Let me know if anything seems wrong.

Note: I use the term nested option to describe options that can hold other options (lists and records in our case).

Changes that we have to make from the current application:

1. Apply transformDoc to the initial doc state (if I’m not mistaken, I don’t have to make any changes to your code to get it working). I’ll make changes if required though.
2. Update types to conform to the new in-memory doc that we’re using.
3. Update reconcileOptions tests. I’ll have update the payloads so that they use the new option typing.
4. Update reconcileOptions if required (I’ll know after changing the tests.
5. Fix exampleDoc to use options that are more descriptive.

Testing

I'm not really sure how to approach testing now that we have unique ids being generated. I considered mocking but I found it would get quite messy to update the ids if we added/removed things to the document in that particular test case. I was thinking we just test the existence of the new values. E.g.

{
      type: "SketchbookDocument",
      layers: [
        {
          ...
          options: {
            ...
            id: "id",
            value: {
              recordExample: {
                id: "bd43ff44-6718-48ae-b4a6-8040a92237ad",
                value: {
                  foo: {
                    id: "664b096f-0316-4928-b44e-08d465054493",
                    value: "bar"
                  }
                }
              }
            }
          }
        }
      ]
    }

We may just assert that

• doc.layers.options.value.recordExample.id exists
• doc.layers.options.value.recordExample.value.foo.id exists
• doc.layers.options.value.recordExample.value.foo.value is equal to its original value
  What do you think of this?

More changes (function handlers)

1. Navigate: use dfs to search for the options to navigate to (params: id)
2. Change the value of a scalar: use dfs to search for the parent of the option being changed and then update the state of the scalar.
3. Delete a scalar inside a nested option: same as above.
4. Add a scalar or a nested option to a nested option: same as above
5. Ability to reorder the nested options: use the Draggable stuff I had from before.

I was thinking about how to recursively handle the behaviours. I’m thinking I’ll have to go through the doc object once to calculate the path to the selected option (a path of ids) before iterating through again to actually navigate to the option or make changes to it (every other behaviour). This is if we want to keep things functional. The alternative method in my eyes is to use a for loop and return early when we come across the object we want.

[haydn]

@Mxchaeltrxn Awesome breakdown of what needs to be done!

It's dawning on me now how much of the app will need to be changed because of those ID's being introduced. I'm thinking we should tackle that first in a separate PR and get it merged into master, then come back to these recursive options.

And that gets me thinking… it might be worthwhile doing a bit of a spike before that to see if Recoil is the path we want to take. Moving across to Recoil would probably be just as disruptive, so we should make a decision on that now and save ourselves doubling up on work if that is the path we want to take. In my understanding of Recoil's model, each option will end up being its own "atom", which should solve some of the problems we're facing here.

I'll write-up a new issue today for a little spike into Recoil (probably just mocking up something in CodeSandbox to test the hypothesis). Would you be happy to give that a shot? Recoil is pretty new and it's probably not very googleable yet, so it might take a bit of investigating/experimentation to learn all its ins-and-outs.

I'm not really sure how to approach testing now that we have unique ids being generated.

Yeah, those nondeterministic ID's can be a pain. Just checking they're present is probably enough. If you want to get fancy you could also check if they're valid:

https://github.com/uuidjs/uuid#uuidvalidatestr

The other thing you could test is if they're all unique. Not sure there'd be much value in that though because it can't be reliably tested (tests might fail one time and then pass the next, depending on how the ID's are being generated).

[Mxchaeltrxn]

@haydn Yeah for sure I'd be happy to take a shot at doing the feasibility study/test!

[haydn]

@Mxchaeltrxn Awesome!

Sorry, didn't get around to it yesterday! Here's the issue, ready to go now: #116