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

Version 0.4 #47

Merged
merged 16 commits into from May 10, 2016
Merged

Version 0.4 #47

merged 16 commits into from May 10, 2016

Conversation

Gozala
Copy link
Contributor

@Gozala Gozala commented May 9, 2016

Overview of the changes

  1. Fix regression introduced by 4e13b24 & ba21bbb that cause misbehaviors on synchronous tasks.
  2. Factor out requestAnimationFrame scheduler from Effects library and expose it via tasks instead. Having it part of Effects did not really made much sense.
  3. Renamed Effects.task(task) to Effects.perform(task) as former was pretty confusing.
  4. Changed API for creating cancellable tasks so they won't perform extra allocations on execution. Most relevant for requestAnimationFrame task that are used a lot during animations.
  5. Change class hierarchies to avoid passing own methods to super calls.
  6. Add Task.prototype.recover API so that task failures can more efficiently & intuitively be translated to error actions.

Following example transform failing task into task that always succeeds with Result<error, value>:

```
new Task(exec)
  .recover(Result.error)
  .map(Result.ok)
```
It made little very little sense that Effects.tick was a special type of Effect. This change moves functionality provided by it into `Task.requestAnimationFrame` instead & provides temporary backwards compatibility with warning via new implementation.
Prior name was confusing especially since it had to be passed a task instance. `Effects.perform(task)` seems to better communicate intents.
Those changes changed finally blocks to catch blocks which meant they would only be executed on exceptions, which was incorrect since finally blocks always execute & in this instances they performed state updates.

More specifically synchronous tasks would no longer run given that messages would be queued into inbox but new messages were only checked on exceptions or when new messages arrived.
@Gozala
Copy link
Contributor Author

Gozala commented May 9, 2016

@tschneidereit r?

driver = previous

if (exception != null) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if render() throws null? Probably not a problem in practice, but at least comment to that effect. If it could be a real problem, then change this to something like

// Somewhere above `renderWith`
const NoExceptionSentinel = {};

// At the top of `renderWith`
let exception = NoExceptionSentinel;

// At the end of `renderWith`
if (exception !== NoExceptionSentinel) {
  ...
}

@tschneidereit
Copy link

Looks good with feedback addressed. I'm not too happy with a new requestAnimationFrame function that works differently from the builtin version, but I'm also not sure what would be a better name. Perhaps something like queueAnimationFrameCallback and removeAnimationFrameCallback or something along those lines.

@Gozala
Copy link
Contributor Author

Gozala commented May 10, 2016

Looks good with feedback addressed. I'm not too happy with a new requestAnimationFrame function that works differently from the builtin version, but I'm also not sure what would be a better name. Perhaps something like queueAnimationFrameCallback and removeAnimationFrameCallback or something along those lines.

@tschneidereit Is that in reference to exports of preemptive-animation-frame module ?

@@ -10,6 +10,7 @@ export type {Text, Key, TagName, DOM}
*/

let driver/*:?Driver*/ = null
const absent = new String("absent")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, much better than my proposed {}.

@tschneidereit
Copy link

@tschneidereit Is that in reference to exports of preemptive-animation-frame module ?

It is, yes.

@Gozala
Copy link
Contributor Author

Gozala commented May 10, 2016

@tschneidereit Is that in reference to exports of preemptive-animation-frame module ?

It is, yes.

I'm afraid I can't think of better name maybe requestPreemptiveAnimationFrame / cancelPreemptiveAnimationFrame ? But then module name kind of already implies that no ? Only difference between native and this implementation is that this one is preemptive, meaning it ensures you don't miss a frame regardless when you requested it.

Given that it's just an internal module (that I actually want to factor out into separate package) I'll leave it as is for 0.4 as I would not like to block a release for this. My interpretation of your comment on this was that it was "nice to have" vs "must have".

@Gozala Gozala merged commit a12e1fd into master May 10, 2016
@tschneidereit
Copy link

Given that it's just an internal module (that I actually want to factor out into separate package) I'll leave it as is for 0.4 as I would not like to block a release for this. My interpretation of your comment on this was that it was "nice to have" vs "must have".

That makes sense.

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.

None yet

2 participants