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

remove setTimeout #1092

Closed
wants to merge 2 commits into from
Closed

remove setTimeout #1092

wants to merge 2 commits into from

Conversation

kofifus
Copy link
Contributor

@kofifus kofifus commented Apr 11, 2022

This PR does two things:

  1. remove the fallback to setTimeout if RAF is not available. In such rare environments RAF can be replaced externally.
  2. move the assignment busy=true to a separate line to increase code readability.

I tested on a few of the available codepen examples simply replacing the hyperapp link with https://raw.githack.com/kofifus/hyperapp/patch-1/index.js

@jorgebucaran
Copy link
Owner

This will cause a runtime error if rAF is not defined in the environment e.g., Node.js. Or will it not?

@kofifus
Copy link
Contributor Author

kofifus commented Apr 12, 2022

Yes it will.

The thinking is that for environments where RAF is not available (which is quite rare for hyperapp), it needs to be replaced outside of hyperapp (which is an easy one liner which should be there for other code that may need RAF). It is not catered for by hyperapp internals.

@zaceno
Copy link
Contributor

zaceno commented Apr 12, 2022

If I read this change correctly, the runtime error in e.g node will only occur when you try to run an app with a view (i.e. headless mode is still safe). If you are using node and want to render a view you already need a simulated browser environment anyway or else there are plenty of things that will throw errors. I’d argue it’s the responsibility of that environment to provide an implementation of rAF just as it provides an implementation of document.createElement et c

in other words: I endorse this change :)

@sergey-shpak
Copy link
Contributor

sergey-shpak commented Apr 21, 2022

@kofifus @zaceno @jorgebucaran please consider prev discussion: #866

@kofifus
Copy link
Contributor Author

kofifus commented Apr 22, 2022

OK seems like this is much more involved then I assume, I think it's better closed for now.

@kofifus kofifus closed this Apr 22, 2022
@zaceno
Copy link
Contributor

zaceno commented Apr 22, 2022

@kofifus @sergey-shpak Hyperapp has changed since that discussion. Effects/subs are no longer deferred. Running headless apps in e.g. node.js does not require a mocked requestAnimationFrame to be available.

I even verified that by using the version of hyperapp proposed in this PR, and running the following code in node with no errors reported and correct output produced.

import { app } from "./hyperapp.mjs"
const Incr = (state) => [state + 1, () => console.log("Incremented ", state)]
const dispatch = app({ init: 0 })
dispatch(Incr)
dispatch(Incr)
dispatch(Incr)

@sergey-shpak
Copy link
Contributor

sergey-shpak commented Apr 22, 2022

@zaceno yes, you are right, since effects are no longer deferred and requestAnimationFrame is used only if view is defined - it makes sense to remove the fallback 👍

@kofifus no need to close this, we are fighting for every improvement here and you are on the right way), the only change I would suggest is to make it one-liner if (view && !busy) requestAnimationFrame(render, (busy = true))

waiting for approval from @jorgebucaran

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.

4 participants