refactor: simplify JSV

[marcelltoth]

Alright, so this is going to be opinionated. Prep your coffee, tea, monster (😉), whatever. Don't read in a bad mood please. 😇

Background

We've long had an issue, stoplightio/elements#667. If you read the description you can see that while it's a non-trivial problem, it feels reasonable, something that can be done.

Then we tried to implement it. We struggled. Then we invited @P0lip to help us. Then we tried again. Struggled still. Then we held a 4-hour long session together to make something useful. We still struggled, but we built some POC. I tried to build something complete myself based on our POC, but I failed. I was running into all sorts of hard-to-debug state-management-related issues, crashes, etc. Items missing from caches, TypeError-s, etc. I gave up.

TLDR

While the current state management approach JSV is using has some wonderful features, it is unfortunately borderline impossible to comprehend by basic human beings like myself. This poses a huge risk to the Elements project (and possibly the Stoplight ecosystem as a whole) because - as this oneOf/anyOf issue shows - Team Undefined seems to be simply incapable of working on it, in it's current form.

Proposal

I propose to remove most of the state-management complexity from JSV, and fall back to a basic, idiomatic react approach:

• No mobx
• No stores of any kind
• No tree-list
• No virtualization
• Much less context usage, much more props passing

The foundation, json-schema-tree is kept as-is, JST is an amazing library for abstracting away the bulk of the logic. There wasn't much extra logic actually on top of what JST offers, the most notable one being array element type hoisting. (a.k.a. "flattening") That logic was moved from the SchemaTreeListTree class to custom hooks and functions, see utils.ts

The bulk of the presentation logic is kept intact, except it is now simpler as it:

• only uses a single source of input: the JST node, no jumping between a tree list node and a JST node
• uses only props for the most part, not a lot of context, which makes components easier to reason about
• some typing improvements were made, see PrimitiveArrayNode, etc

Results

The good

1. It's hopefully much simpler to understand and work on. There is evidence of this as I have built a rudimentary version of the oneOf/anyOf selector in only about 70 LOC (and in about 2 hours) which already works much more reliably than our old POC.
2. It unknowingly fixed some bugs. Come here open Permissions then ids. It's broken, I found it when comparing the new with the old. The new version just works. It's much more primitive logic, but because of that, there's likely less edge cases and random bugs like this.
3. Removed ~400 LOC, improving readability and bundle size.
4. Bundle size is much better. Own size is 12KB vs 20KB prior, in addition to losing two big dependencies:
   • tree-list, which is 37KB minified
   • mobx & mobx-react-lite which are 57 KB and 6 KB respectively.
5. Not having virtualization soles a lot of our current and future problems, such as:
   • Struggling to instantiate JSV into a non-fixed-height container.
   • Not being able to show the entire description if the user asks for it. (Word wrapping and virtualization don't play well together.)
   • Being able to render and assert against JSV in a JSDOM environment without mocking either JSV or autosizer or both.
6. stoplightio/platform-internal#5476 should be implicitly resolved by this change.

The bad

1. Initial rendering performance is worse for large schemas, especially if defaultExpandedDepth > 1.

The stress test which renders a 15-thousand-line json-schema twice with defaultExpandedDepth: 2 takes a little more than 1 second on my machine vs the virtualized version, which takes about 300ms.

I would say this is still definitely the "worth it" category. The results above are without any attempt at performance optimization, but even if we can't improve it much: 30k lines worth of content loading in 1 sec should be acceptable IMO. We'll throw a loading-spinner, animation or anything and it will feel butter-smooth. (Or a progress-bar, right, @wmhilton? 😉)

TODO

1. While working on the POC I removed some props, some of which may be needed, I need to add them back.
2. As tests were largely relying on Snapshots and props passing, I need to update and/or rewrite them.

[mpodlasin]

For me this is insanely impressive.

However I am obviously waiting for the passing tests - my main gripe would be catching any regressions, so final opinion on this PR would largely depend for me on quality of test refactor.

[mallachari]

I'm surprised that removing tree list went so smoothly. I was expecting much bigger refactor. At this point the code is way more accessible and considering current requirements that change might be good to have. I got used to tree list usage, also got familiar with it to some extent, but trying to add recent changes made it too painful and unnecessarily complex. In the end it's a matter of picking between virtualization and simplicity.
I didn't notice anything breaking with this approach so far, though porting tests might encounter something that was missed.
Generally I like this approach as it solves problems that we are struggling with for some time, though I'm curious of the opinion from the void side.

[P0lip]

I'm surprised that removing tree list went so smoothly. I was expecting much bigger refactor. At this point the code is way more accessible and considering current requirements that change might be good to have. I got used to tree list usage, also got familiar with it to some extent, but trying to add recent changes made it too painful and unnecessarily complex. In the end it's a matter of picking between virtualization and simplicity.

@mallachari yeah, it's probably because the last time you looked at the component, JSV was strictly tied to tree-list.
Plenty of UI components accessed tree-list nodes directly (its metadata), and the whole tree was somewhat built on top of tree-list's tree (we simply created tree-list tree nodes when iterating over schemas, etc.)
It was painful as hell to get rid of that tree-list usage. I had to spend 2-3 days to do that, but had to do it, so that you all don't hate me
Once JSV was set to use JST, the tree-list was actually used in one spot, that was still quite tangled, but there was not much I could do about it, cause we still had to map JST node with a tree-list node and so on.

---

Speaking of the PR, I'm going to review it soon, probably today or tomorrow.

[marcelltoth]

@mallachari yeah, it's probably because the last time you looked at the component, JSV was strictly tied to tree-list.
Plenty of UI components accessed tree-list nodes directly (its metadata), and the whole tree was somewhat built on top of tree-list's tree (we simply created tree-list tree nodes when iterating over schemas, etc.)
It was painful as hell to get rid of that tree-list usage. I had to spend 2-3 days to do that, but had to do it, so that you all don't hate me
Once JSV was set to use JST, the tree-list was actually used in one spot, that was still quite tangled, but there was not much I could do about it, cause we still had to map JST node with a tree-list node and so on.

Oh yeah, I wouldn't have attempted this a few months ago 😂 . The JST-extraction was amazing, it took out most of the core logic without having any UI-related dependencies. This is philosophically meant to be a follow-up to that PR, completing what you started there.

[P0lip]

Yeah, to be frank, when working on #91 and related PR, I had a moment when I thought it might be easier to simply write a new component.

---

Speaking of dropping virtualization, I'm totally down for it, as long as it doesn't meaningfully impact performance.
I know very well how difficult it was to deal with it, therefore I totally understand your motivation.
In any case, I'll take a look at it soon, but I doubt I'll have any major objections unless any of the test models I use for testing turns out to be slow to render, yet hopefully that's not the case.

[domagojk]

The PR looks great! So much more deletions than insertions 🤩 But I haven't look into detail, I just wanted to say that this PR will automatically close this one:

And I guess we shouldn't do this because we still need to update it in studio 🤷‍♂️

[marcelltoth]

Oh, these magic words always get me, even though I know them 😄 Thanks for the heads-up @domagojk, I'll update it.

[marcelltoth]

I'm okay with dropping tree list, therefore I'm also fine with the chosen approach
Ping me when this PR is ready and I'll be happy to have a final pass.

@P0lip I cleaned up the TODOs, addressed your comments and reimplemented the required props with one change: I swapped the custom rowRenderer feature for a rowAddon that does pretty much the same and fits the masking use-case just as well. Reasoning being that with SchemaRow now rendering its own children, writing a correct and good looking rowRenderer would be tricky I found out when trying it out in the storybook. This narrowed feature-set still serves the same purpose but should be much easier to use.

The only TODO left is the tests, which are being handled by #111 first, otherwise this is ready for a next pass.

[P0lip]

Looks good with the exception of stepIn being gone in the new code.
We should try to add it back. LMK if you have any concerns
Apologies I didn't spot this during the first pass.

[P0lip]

You lost stepIn in the transition.
This is quite important, otherwise really large schemas with a ton of $refs will be much slower to process than needed.
stepIn is needed, so that JST tree is not built upfront for the whole schema, but for actual parts that are currently rendered.
This is also one of the more notable factors contributing to the entire complexity of the former solution.
While, getting rid of virtualization is fine by me, we certainly should at least try to preserve that optimization.
By adding it back, we should be more or less on par with current v3.

[P0lip]

Hmm, or maybe not.
This is what happens when I try to open one of our reference models. (scroll to the bottom of my comment, it's not an issue caused by your PR, but a relatively minor issue in JST)

Chromium is no different. 😢

Note - I modified the story to load only a single instance of JSV instead of two.

 .add('stress-test schema', () => (
    <>
      <div style={{ height: '100vh', overflowY: 'scroll' }}>
        <JsonSchemaViewer
          schema={stressSchema as JSONSchema4}
          defaultExpandedDepth={number('defaultExpandedDepth', 2)}
          onGoToRef={action('onGoToRef')}
        />
      </div>
    </>
  ))

It's likely to be something off with JST meeting some very recursive data, as the model I test it against has plenty of such.
I'll look into it now.

---

I got the fix for that JST issue, but even with that, it's still quite slow.
With profiling on and no CPU throttling it takes over a second for render, which is probably too much.
With CPU throttling set to 4x, it's over 7s.
Do note that I still test it having the above storybook tweak applied.

Now, in the case of current origin/beta tip the results are ~250ms and ~750ms respectively for CPU 4x throttling.

Whether it's acceptable or not, it's up to Product team, I suppose.

[P0lip]

Aside from that perf issue, the code is good 👍

[P0lip]

Do we still rely on UI-Kit in JSV?

[marcelltoth]

Unfortunately it's still a peer of MDV :\

[marcelltoth]

I removed the include from the storybook _styles.scss. That at least make Storybook rebuilds faster.... which is a win I guess... 😅

[stoplight-bot]

🎉 This PR is included in version 4.0.0-beta.4 🎉

The release is available on:

• npm package (@beta dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[stoplight-bot]

🎉 This PR is included in version 4.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀