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

Upgrade to React 18.2 and add thin cljs hooks layer #242

Merged
merged 5 commits into from
Oct 28, 2022
Merged

Upgrade to React 18.2 and add thin cljs hooks layer #242

merged 5 commits into from
Oct 28, 2022

Conversation

mk
Copy link
Member

@mk mk commented Oct 24, 2022

This upgrades React to 18.2 and adds a thin layer over react hooks for more ergonomic cljs usage:

  • the fn passed to useEffect returns js/undefined for any non-function value
  • hooks that can take a deps array default to an empty array, and convert vectors/sequences to arrays
  • useState and useRef return values that behave like atoms
Warning: useEffect must not return anything besides a function, which is used for clean-up.

It looks like you wrote useEffect(async () => ...) or returned a Promise. Instead, write the async function inside your effect and call it immediately:

useEffect(() => {
  async function fetchData() {
    // You can await here
    const response = await MyAPI.getData(someId);
    // ...
  }
  fetchData();
}, [someId]); // Or [] if effect doesn't need props or state

@mk
Copy link
Member Author

mk commented Oct 24, 2022

@borkdude
Copy link
Collaborator

@mk Async function in JS just means:

  • returns promise
  • you can use async/await syntax

You can achieve the same in SCI using promesa.

@mk
Copy link
Member Author

mk commented Oct 24, 2022

@borkdude did you see the error above? Not sure what the differences are, we are currently returning a promise but it suggests It looks like you wrote useEffect(async () => ...) or returned a Promise. Instead, write the async function inside your effect and call it immediately.

@borkdude
Copy link
Collaborator

@mk Can you show me where that error is? I'll be able to take a deeper look in an hour or so

@mk
Copy link
Member Author

mk commented Oct 24, 2022

@borkdude you get it locally on the console when showing the vega or plotly sample notebook.

@borkdude
Copy link
Collaborator

@mk Right, I don't know if there is a flag to "toggle" a function's asyncness, but wrapping it inside an async function which awaits the inner promise might work then.

@borkdude
Copy link
Collaborator

This hack may also work:

cljs.user=> (def sci-fn (sci/eval-string "(fn [] (js/Promise.resolve 10))" {:classes {'js js/globalThis :allow :all}}))
#'cljs.user/sci-fn
cljs.user=> (def async-constructor (js/eval "(async function () {}).constructor"))
#'cljs.user/async-constructor
cljs.user=> (set! (.-constructor sci-fn) async-constructor)
#object[AsyncFunction]
cljs.user=> (sci-fn)
#object[Promise [object Promise]]

@mhuebert
Copy link
Contributor

@mk I believe you're seeing that error because useEffect should either return a function or js/undefined. In the React doc examples, they are calling their async function at the end of their useEffect function but since it's JS there is no implicit return so the function returns undefined.

@mhuebert
Copy link
Contributor

mhuebert commented Oct 27, 2022

In 95b0db1 I added a thin layer over react hooks for more ergonomic cljs usage:

  • the fn passed to useEffect returns js/undefined for any non-function value
  • hooks that can take a deps array default to an empty array, and convert vectors/sequences to arrays
  • useState and useRef return values that behave like atoms

@mk
Copy link
Member Author

mk commented Oct 27, 2022

@mhuebert looks fantastic, thank you!

# Conflicts:
#	resources/viewer-js-hash
@mk mk changed the title Upgrade react to 18.2 Upgrade to React 18.2 and add thin cljs hooks layer Oct 28, 2022
@mk mk merged commit 10691bf into main Oct 28, 2022
@mk mk deleted the react-18 branch October 28, 2022 07:57
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