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

Improve error handling & refresh for with-d3-require #231

Merged
merged 13 commits into from
Oct 20, 2022
Merged

Conversation

mhuebert
Copy link
Contributor

@mhuebert mhuebert commented Oct 14, 2022

Addresses the issues of

  1. d3-require staying mounted & not losing its height when re-rendering/refreshing
  2. how to handle async errors in a :ref function.
  • Use reagent functional components for sci-viewers
  • error-boundary provides :!error as react context
  • use-handle-error hook so that child views can write to the nearest error handler manually
  • add use-d3-require hook
  • simplify with-d3-require: no need for a :key - the component stays mounted and re-fetches packages whenever the args change. the invalidation now occurs where we pass deps to useEffect

@mhuebert mhuebert force-pushed the d3-require-wrap-callback branch from e91208b to 6e7b94b Compare October 14, 2022 09:25
@mhuebert
Copy link
Contributor Author

After a chat with @mk tried another route - using hooks and a function component. It's not particularly ergonomic to do with reagent but much easier to achieve exactly what we want.
Fixed the issue with the height of the view changing in-between renders. Now it accepts :refresh-key to determine when to re-render.

But the original issue - async error catching - is not handled in this version, we now pass our !error atom down via react context, so we should access that and use it with the ref callback. Didn't have time yet.

@mhuebert
Copy link
Contributor Author

I added the async error handler back in, using a hook, and to simplify things - I changed the default reagent compiler to functional components. So this would now need more testing. But supporting hooks IMO is very useful.

@mhuebert mhuebert changed the title with-d3-require returns wrap-callback fn for :ref error handling with-d3-require - refresh + error handling Oct 14, 2022
- use-promise hook
- remove support for :then in with-d3-require
- :refresh-key no longer needed
@mhuebert
Copy link
Contributor Author

further re-implemented everything using hooks, much simpler and composed of small parts.

@mk
Copy link
Member

mk commented Oct 15, 2022

Very nice. There still seems to be an issue with Plotly in the static build still, see https://snapshots.nextjournal.com/clerk/build/88e94fce8b12fd4c6770ecfea60f616fbb00388f/index.html#/notebooks/viewers/plotly.clj

Locally I saw an Rendered more hook than on the previous render error, but I can't reproduce it reliably not sure if that's related or just caused by me pulling from a state without hooks.

@mhuebert
Copy link
Contributor Author

@mk I also see that hook error locally when switching between different notebooks. Looking into it.

@mhuebert
Copy link
Contributor Author

I found two issues -

  • an advanced-compile issue that was fixed by adding a return-type hint to use-d3-require,
  • an issue with how inspect-presented was calling viewer functions, which wasn't giving each "viewer" its own React component instance (& therefore, its own hook state). Fixed by adding a 3rd arity to inspect-presented which is invoked as a component with ^{:key viewer} so that viewers get hook state which is persisted across re-render. b883522#diff-2eb22f30e4f63ae4ccc753c326e323e3cd1b0bd273ca1655399f12b033da2a64R570

@mk
Copy link
Member

mk commented Oct 20, 2022

@mhuebert just getting around now to play with this, it's SO much better than what we had before, both in experience (without the height flickering) and code. Would love to go over the inspect-presented change on a call but that can happen independently of the merge.

# Conflicts:
#	resources/viewer-js-hash
@mk mk changed the title with-d3-require - refresh + error handling Improve error handling & refresh for with-d3-require Oct 20, 2022
@mk mk merged commit 6e7f103 into main Oct 20, 2022
@mk mk deleted the d3-require-wrap-callback branch October 20, 2022 12:06
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 this pull request may close these issues.

3 participants