Refactor graph rendering from closures into a class #398

Iftahh · 2020-12-29T11:04:55Z

I refactored the rendering code, instead of one function with many local variables and inner functions (ie. closures) it is a class with methods and class variables.

This change enables swapping the class for another one (as an optional argument to createGitgraph), where different methods can be overridden, thus enabling extending and changing different elements in the rendered SVG.

The rendering code is a bit less elegant because you have to use this.variable instead of variable,
and when passing a function to map(func) you have to pass map(this.func.bind(this))
but otherwise, the code is nearly identical, and much more open to changes, either by the library writer (eg. different layouts can be different Renderer subclasses) or by the client (eg. pass a custom renderer to change different methods).

Other very small changes I made was utility cached function to get the parent commits (because I wanted to use it multiple times) and a utility function to move an element by X and Y.
I used this to make changes to the rendering of the graph and it worked very nicely.

packages/gitgraph-js/src/gitgraph.ts

crutchcorn · 2020-12-30T20:58:00Z

The rendering code is a bit less elegant because you have to use this.variable instead of variable,
and when passing a function to map(func) you have to pass map(this.func.bind(this))

Would this make this PR a breaking change, then? Does the consumer have to change their code?

and much more open to changes

Are you able to give a demonstration on a use-case that would benefit from something like this? I don't doubt you by any means, just having difficulty picturing it in my head immediately

On a tangential note, I find it funny that shortly after I start refactoring the React code away from classes into functional components. That's not to say classes don't have their merits or anything - simply an incidental of two people getting involved in the codebase around the same time that's humorous in a small way

nicoespeon

Hey @Iftahh 👋

That's a good chunk of work you did here, thank you for that 👍

Overall, I don't really mind if the source code follows more of a functional or an OO approach. In JS, both are do-able. TS gives a bit more room for OO style, that's for sure. But you can do a decent FP style with it too.

I understand what you're aiming for: being able to replace parts of the rendering process by extending the Renderer class. This is actually achievable with a functional style too: you'd pass rendering functions in place of the methods, or they would default of the base behavior. Converting to a class isn't necessary to achieve that. But using classes to achieve that works well too.

As @crutchcorn said, it's indeed a bit funny to see 2 different style being proposed to the codebase:

@gitgraph/react is going for a more functional style (it was using classes)
@gitgraph/js would be going for a more OO style (it was functions)

I'm totally fine with it though. But I'm not maintaining the codebase anymore, so I would let @crutchcorn (and other active maintainers, if any) have a word on this.

@crutchcorn is raising a fair point: is this a breaking change for code consumers? I don't think so from what I see: we're adding a new optional renderer parameter, preserving existing behavior. I can see that all tests are passing, meaning that all the visual scenarios still work as before with the same code. Therefore, I'm good with the change. Did I missed something @Iftahh ?

Also, I left a couple of comments I'd love you address:

Unexpected changes in the import spaces. I thought we had Prettier configured as a pre-commit hook. In any case, please make sure to adapt the code to the current formatting style to avoid unnecessary changes and conflicts
Let's add a few tests to illustrate the new renderer parameter of the createGitgraph() function and prevent future regressions.

Thank you again 👍

nicoespeon · 2021-01-01T17:58:00Z

packages/gitgraph-core/src/commit.ts

+import {Branch} from "./branch";
+import {Refs} from "./refs";
+import {Tag} from "./tag";
+import {GitgraphTagOptions} from "./user-api/gitgraph-user-api";


It seems some unexpected reformatting happened on these files. Could you revert these changes? I don't think these were expected, are they?

Sure - I'm sorry for these accidental changes (my IDE is configured for making these and I didn't notice), I'll revert.

Still not fixed, I guess that's because you can't get it set up locally. We need to fix this so you can develop in good conditions =)

packages/gitgraph-js/src/gitgraph.ts

nicoespeon · 2021-01-01T18:24:34Z

packages/gitgraph-js/src/gitgraph.ts

+  const gitgraph = new GitgraphCore(options);
+  if (renderer) {
+    renderer.gitgraph = gitgraph;
+  }


On a different note, but related to previous comment: I'd suggest making Renderer an interface that SvgRenderer currently implements.

Not 100% sure this is necessary, but at this point it seems that we could accept any kind of Renderer implementation that wouldn't necessarily use SVG to render the graph.

Now I said I'm not 100% sure because it adds an abstraction and I'm not sure we need it at this point. The JS library was designed to use SVG and we didn't expected end-users to change that (or they would have created a different rendering library instead). But I can see some value here. Again: I'm not sure because I don't maintain this anymore, so I'll let that decision to you, current users of the lib 👐

nicoespeon · 2021-01-01T18:26:30Z

packages/gitgraph-js/src/gitgraph.ts

+function createGitgraph(
+  graphContainer: HTMLElement,
+  options?: GitgraphOptions,
+  renderer?: Renderer,


I can see we're adding the possibility to provide a custom renderer, but there's no test associated that would present the new capabilities.

Could you add one to illustrate what is possible to do?

I think that will help explain the benefits of such a change. Also, it will make sure we don't break these new features next time we touch the code (e.g. we refactor the Renderer class and break the public API without realizing it).

Iftahh · 2021-01-02T00:51:23Z

hi, thank you both for reviewing and for your comments.

First, I apologize for not responding earlier, I'm on a family vacation and have even less free time than usual.
Second,
I made this change for myself because I wanted to modify slightly the rendering and found no easier way to do it. Only as an afterthought I figured this may be something of interest to the greater community, so I published this as is. This is my "excuse" for not discussing prior to coding, and why I did not write unit-tests, and even more shamefully, I did not set up the e2e tests so I didn't ran the full pre-commit hook (which is how the imports order got messed up).

The use case I was solving are a few visual changes: I have very limited horizontal space so I wanted the tags to be above the "message" - ie. I had to move the message down instead of to the right. I also wanted to add a "rect" element at the background, and a css class on the commit "g" so that I can change the rect color on hover, and select (ie. add a class) it on click. This rect element has to have a specified height, so I had to have access to the calculated "Y" coordinates. These changes are tiny in the source code but I had no way of accomplishing it with the functions being internal closures.

I'll add some tests and a storybook example of how to use it, when I get back to work (~Monday)

Iftahh · 2021-01-02T01:22:03Z

As you understood, there are no breaking changes (that I know of) and any existing integration should work just the same with this change.

However, there are a couple of downsides I can think of:

This change does expose the internal functions (and variables!), so future changes may be more difficult to do without breaking integrations.
I'm not sure this is an issue, but developers who want to support old browsers may have a harder time to override class methods in vanilla JS (it is possible, and can be documented to help such developers).

Using a custom Renderer to override functions can definitely be labeled as "advanced" usage, and can be documented with a "use at your own risk, future API may change" warning, so both points are not huge downsides in my opinion. It may be possible to reduce visibility (from "protected" to "private") of several methods, limiting the danger of creating future problems with backward compatibility. I did not consider exactly which functions would be the most beneficial to expose, and rather exposed many of them without restraint.

Regarding using a more functional style, ie. callback functions instead of overriding class methods - I believe there are problems with this approach because sometimes you want a base function to call the overridden function, and sometimes you want the custom function to call the base function, and this sort of things come naturally to the object-oriented developer but can get confusing in the functional paradigm. It worked nicely for the "renderMessage" callback, because the render message isn't calling any other functions, but I don't think it will be quite so easy for other elements. I afraid to do it properly without classes you will end up re-inventing the class solution (ie. calling super().method()) but with uglier syntax. That said, I could be wrong here, and maybe it will be easier than I anticipate.

Iftahh · 2021-01-08T22:02:31Z

I made a few changes:

changed the code to use the createRenderer that @nicoespeon suggested.
instead of setting svg.innerHTML to "" and adding a child, I use replaceChild (if the browser supports it) - this prevents some flicker and preserves the scrollbars to where they were before the re-render.
added "rerender" method to render the diagram again with the same data - maybe it should be part of the "public" API and not the renderer method.

I added two stories to the Storybook - they are the last two in "chapter 6"
one showing a customized positionCommitsElements to move the message down below the tags,
and the other for adding "rect" to the background of commits which changes colors depending on selected/hover.

I still haven't managed to get the project setup 100% functional on my laptop - I gave it a bit more try today and gave up. I may need your help there. Because of this I can't run the pre-commit hook and don't have the full tests suite running.

This will enable swapping the class for another one, where different methods will be overridden, thus enabling extending and changing different elements in the rendered SVG. Added 2 Storybook stories for custom Renderer class - one showing a customized positionCommitsElements to move the message down below the tags, and the other adding "rect" to the background of commits which changes colors depending on selected/hover.

nicoespeon · 2021-01-12T22:24:29Z

First, I apologize for not responding earlier, I'm on a family vacation and have even less free time than usual.

Well, you don't have to worry/apologize on that! This is something you do on your free time, do it as you want/can. No pressure!

I have very limited horizontal space so I wanted the tags to be above the "message" - ie. I had to move the message down instead of to the right. I also wanted to add a "rect" element at the background, and a css class on the commit "g" so that I can change the rect color on hover, and select (ie. add a class) it on click.

I understand the rationale and I think you're not the only one who'd like such features, so it makes sense 👍

As you understood, there are no breaking changes (that I know of) and any existing integration should work just the same with this change.

If CI is green, then it means there's no regression we're aware of 👌
If there's a regression, then the process is to add a test to cover that and release a fix.

I'll review the changes you made.

Thanks for the new stories, they illustrate the use-cases well indeed 👏
However, I'm not sure if the second one behaves as expected. Here's the screenshot I can see:

The content seems to be cropped on the right. Is it normal?

nicoespeon · 2021-01-12T22:26:40Z

I still haven't managed to get the project setup 100% functional on my laptop

Hmmm, this is a problem. What is not working exactly? Do you have a specific error?

nicoespeon · 2021-01-12T22:28:59Z