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

Isolate m.prop() and m.deferred() implementations (mostly) #853

Merged
merged 7 commits into from
Dec 15, 2015

Conversation

dead-claudia
Copy link
Member

This mostly isolates the implementations for both of these. Now, everything
here calls the method itself, not any of the external methods.

Few driveby fixes as well:

  1. Git now ignores archive/ again (it's a build artifact, and can be removed
    when updating master)
  2. Since I had to rewrite most of the Deferred implementation, the new version
    passes one of the skipped tests, so it is now enabled.

Prepares for #800.

impinball added 2 commits November 20, 2015 02:49
This mostly isolates the implementations for both of these. Now, everything
here calls the method itself, not any of the external methods.

Few driveby fixes as well:

1. Git now ignores archive/ again (it's a build artifact, and can be removed
   when updating `master`)
2. Since I had to rewrite most of the Deferred implementation, the new version
   passes one of the skipped tests, so it is now enabled.
@Naddiseo
Copy link
Contributor

@IMPinball, what are the performance implications of this?

@dead-claudia
Copy link
Member Author

About the same. The bottleneck is still in the rendering, which probably
needs rewritten to reduce GC pressure.

On Fri, Nov 20, 2015, 11:50 Richard Eames notifications@github.com wrote:

@IMPinball https://github.com/impinball, what are the performance
implications of this?


Reply to this email directly or view it on GitHub
#853 (comment).

@dead-claudia
Copy link
Member Author

Promises should be slightly faster, but I haven't benchmarked it much.

On Fri, Nov 20, 2015, 19:16 Isiah Meadows impinball@gmail.com wrote:

About the same. The bottleneck is still in the rendering, which probably
needs rewritten to reduce GC pressure.

On Fri, Nov 20, 2015, 11:50 Richard Eames notifications@github.com
wrote:

@IMPinball https://github.com/impinball, what are the performance
implications of this?


Reply to this email directly or view it on GitHub
#853 (comment).

@dead-claudia
Copy link
Member Author

I would also like to note that this is slightly breaking. m.prop() no longer returns a Mithril promise when wrapping a third-party promise, which could break those depending on this behavior.

@barneycarroll
Copy link
Member

That sound like a passing assertion :)

What happens in author-land resulting in user error with 3rd party integration should stay there. Mithril already documents its disparities with the spec. There is only so much protection you can reasonably offer!

impinball added 2 commits November 26, 2015 07:44
Note that this does *not* memoize `then`, another deviation from spec, but
it's unlikely this will actually cause very many bugs. It's also a breaking
change.
@dead-claudia
Copy link
Member Author

@barneycarroll And a conforming Promises/A+ implementation would see the m.prop() as a thenable, and would grab the then out of it. If the promise implementation raises a fuss over that, it's a problem with the implementation.

Another breaking change: m.then() doesn't return a memoized value (which it shouldn't). It's another deviation from spec, but it's the expected behavior - you're changing the internal value of the thenable when you call the function.

@barneycarroll
Copy link
Member

:shipit:

@dead-claudia
Copy link
Member Author

/cc @lhorie

var prop = m.prop()
promise.then(prop)

prop.then = function (fufill, reject) {
Copy link
Member

Choose a reason for hiding this comment

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

typo... and also why not resolve

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't really know

@dead-claudia
Copy link
Member Author

Merging now. Looking at it again, I don't think that it's a broad enough use case yet that the breaking change would cause problems.

dead-claudia pushed a commit that referenced this pull request Dec 15, 2015
Isolate m.prop() and m.deferred() implementations (mostly)
@dead-claudia dead-claudia merged commit e63a6f3 into MithrilJS:next Dec 15, 2015
@dead-claudia dead-claudia deleted the prop-fix branch December 15, 2015 11:41
@tinchoz49
Copy link

really nice work @isiahmeadows !

@dead-claudia
Copy link
Member Author

@tinchoz49 Thanks!

@dead-claudia dead-claudia mentioned this pull request Dec 15, 2015
8 tasks
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.

5 participants