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

Expose hooks api to render fns, resolve key issues and possibly simplify #238

Closed
mk opened this issue Oct 22, 2022 · 2 comments
Closed

Expose hooks api to render fns, resolve key issues and possibly simplify #238

mk opened this issue Oct 22, 2022 · 2 comments
Assignees

Comments

@mk
Copy link
Member

mk commented Oct 22, 2022

React Keys

With #231 specifically b883522 we changed inspect-presented so each render-fn becomes its own component allowing it to contain hook state. Since then, the following key warnings show up:

Warning: Encountered two children with the same key, `{:name :map-entry, :render-fn #nextjournal.clerk.viewer.ViewerFn{:form (fn [xs opts] (v/html (into [:<>] (comp (v/inspect-children opts) (interpose " ")) xs))), :f #object[sci$impl$fns$fun_$_arity_2]}, :page-size 2}`. 
Keys should be unique so that components maintain their identity across updates. Non-unique keys may cause children to be duplicated and/or omitted — the behavior is unsupported and could change in a future version.

I feel like performance got worse but I didn't get around to measuring it yet. In #237 I've changed the key to also include the :idx which I set in inspect-children but we still get warnings when rendering a table with multiple rows.

In general I'm wondering if we can compute good keys on the JVM to use them in react. It feels right that a key should be derived from the viewer, should it also include an index in collections? I don't think it makes sense to give our leaf nodes an identity and since these lists typically won't get resorted the typical warnings about not using indices for keys do not apply here, correct?

For the code blocks and results, we already generate stable ids that would be suitable for react keys, derived from either the var-name or the hash of the content.

v/html

Another possibly related issue: render-fn's currently always specify a viewer it's always v/html.

(clerk/with-viewer {:render-fn '(fn [x] [:h1 x])}
  "hi there")

If you forget v/html like in the example above, you get a bad error.

unusable viewer `nil`, value `[:h1 "hi there"]`

I believe we should try simplifying and dropping this requirement.

Is there overhead in creating hundreds of react components per result? The map viewer will for example currently create three viewer components per map-entry (one for the map-entry, one for the key & value).

If that is a problem, should render functions have the ability to opt in or out of this?

I'm also interested in an api now that enables us switching to yawn later. Would a switch of compiled vs interpreted hiccup mode be sensible here?

Expose Hooks API via sci-env

Lastly we want to expose a hooks api via the sci env.

@mk
Copy link
Member Author

mk commented Oct 22, 2022

#239 is a first attempt towards simplifying the render-fns dropping the v/html requirement.

While working on it I did run into Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in a useEffect cleanup function. I need to read up on hooks to understand this.

@mk
Copy link
Member Author

mk commented Feb 5, 2023

This has been fixed with #257.

@mk mk closed this as completed Feb 5, 2023
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

No branches or pull requests

2 participants