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

Implements Tracker.Computation#flush and #run (fixes #4514) #4710

Merged
merged 1 commit into from May 12, 2016

Conversation

@deanius
Copy link

@deanius deanius commented Jul 9, 2015

For, #4514, this is my first pass at a core package PR, so feedback welcome!

@rclai
Copy link

@rclai rclai commented Jul 9, 2015

Is it necessary to invalidate()?

@deanius
Copy link
Author

@deanius deanius commented Jul 9, 2015

Tests will fail if you take it out, yes. If you trace execution through _recompute, invalidation is required for it to be recomputed.

@rclai
Copy link

@rclai rclai commented Jul 9, 2015

Oh I see.

@mitar
Copy link
Collaborator

@mitar mitar commented Jul 9, 2015

See alternative implementation at #4715.

@deanius deanius force-pushed the deanius:computation-run-added-4514 branch Jul 10, 2015
@deanius
Copy link
Author

@deanius deanius commented Jul 10, 2015

#4715 has been folded into this PR, and tests covering it have been added. @mitar - does this implementation work for you?

@mitar
Copy link
Collaborator

@mitar mitar commented Jul 10, 2015

Looks great!

@deanius deanius changed the title Implements Tracker.Computation#run (fixes #4514) Implements Tracker.Computation#flush and #run (fixes #4514) Jul 11, 2015
@glasser
Copy link
Member

@glasser glasser commented Jul 14, 2015

Thanks for the feature request! We welcome discussions about how to make Meteor better. If you haven't yet, check out our wiki page about feature requests on GitHub.

I understand that this would be a useful feature, but I'm loathe to just quickly merge it without careful analysis, since it fundamentally changes the invariants about how tracker uses a queue to handle recomputations.

@deanius
Copy link
Author

@deanius deanius commented Jul 15, 2015

So the concern is that it could no longer be taken as a given that recomputations are done in the order they were invalidated in. Well the model of understanding doesn't really change- it's still a queue, but manual line-jumping would be allowed. Perhaps if the computation were taken out of the recompute queue as well ?

@apollo-cla
Copy link

@apollo-cla apollo-cla commented Jul 16, 2015

@deanius: Before we can merge your pull request, you'll need to sign the Meteor Contributor Agreement: https://contribute.meteor.com/

@deanius deanius force-pushed the deanius:computation-run-added-4514 branch Jul 22, 2015
@mitar
Copy link
Collaborator

@mitar mitar commented Sep 12, 2015

Can this get merged in?

@mitar
Copy link
Collaborator

@mitar mitar commented Sep 15, 2015

@stubailo, could also this be merged in? ;-)

@tmeasday
Copy link
Contributor

@tmeasday tmeasday commented Oct 29, 2015

I'd like to add a small 2c here pointing out that in general relying on Tracker re-computations to happen in an order you'd expect can lead to surprising results anyway (e.g. iron-meteor/iron-router#576 (comment))

@mitar
Copy link
Collaborator

@mitar mitar commented Oct 30, 2015

Yes, but we have been using this for few months now and it does help make computations a bit more predictable.

@tmeasday
Copy link
Contributor

@tmeasday tmeasday commented Oct 30, 2015

Oh, no I was supporting your argument and providing a counter to @glasser's point, is all. Just an anecdote of course.

@mitar
Copy link
Collaborator

@mitar mitar commented Oct 30, 2015

;-)

@stubailo
Copy link
Contributor

@stubailo stubailo commented Oct 30, 2015

Perhaps if the computation were taken out of the recompute queue as well?

This sounds like a good idea to me, otherwise using flush or run would result in at least 2 re-runs guaranteed, right?

@mitar
Copy link
Collaborator

@mitar mitar commented Oct 30, 2015

But you have to keep it in the queue because maybe the recomputation again invalidated the queue itself.

There is already a check in _recompute which checks if a computation is invalidated or not. If it is not, it will skip it. So leaving it in the queue I think makes code much cleaner, but the result is the same. On the other hand, if it is invalidated, it is already there.

Or are you saying that we should remove it from the queue and if it gets invalidated again it should be put at the end of the queue? That would be pretty tricky to implement? Mingling with the queue?

@stubailo
Copy link
Contributor

@stubailo stubailo commented Oct 30, 2015

No, I think you're right - I didn't look at it in enough depth.

@mitar
Copy link
Collaborator

@mitar mitar commented Oct 30, 2015

So it will not be rerun twice, but it will be tried to be run the second time.

So the only thing I see is that if it does got invalidated in meantime again, then it will be in the original order, not in the new order. But I also even think that this is OK. From outside it will just look like the computation was run wherever was meant to be run initially. That extra "flush" somebody called on it was out of band anyway.

@stubailo
Copy link
Contributor

@stubailo stubailo commented Oct 30, 2015

I'd like to get one more pair of eyes on this to confirm that it's not going to explode some internals - perhaps @avital?

@zol
zol reviewed May 10, 2016
View changes
packages/tracker/tracker.js Outdated
@@ -353,6 +353,36 @@ Tracker.Computation.prototype._recompute = function () {
}
};

// http://docs.meteor.com/#computation_flush

This comment has been minimized.

@zol

zol May 10, 2016
Contributor

Please remove this comment as docs have been refactored.

@zol
zol reviewed May 10, 2016
View changes
packages/tracker/tracker.js Outdated
self._recompute();
};

// http://docs.meteor.com/#computation_run

This comment has been minimized.

@zol

zol May 10, 2016
Contributor

Please remove this comment as docs have been refactored.

@zol
Copy link
Contributor

@zol zol commented May 10, 2016

Hey guys,

We had another look at this old PR and are happy to merge it. As we have a new docs system, I've commented on the code to remove references to the old docs.meteor.com (referenced via two code comments). Please rebase this PR against the latest devel, make sure our CI tests pass (this should now be working) and we'll be good to go.

@zol zol self-assigned this May 10, 2016
@mitar
Copy link
Collaborator

@mitar mitar commented May 11, 2016

Great news. @deanius, do you have time to look into this?

@deanius
Copy link
Author

@deanius deanius commented May 11, 2016

Wow, yeah @mitar, I'll make some time for it this evening.

@deanius deanius force-pushed the deanius:computation-run-added-4514 branch May 11, 2016
@deanius
Copy link
Author

@deanius deanius commented May 11, 2016

Hey @zol- you want just those 2 comments removed, right? Let me know if you want this squashed or anything. -Dean

@zol
Copy link
Contributor

@zol zol commented May 11, 2016

Yes, just those two comments removed would be great. We've had a lot of PR activity since yesterday's issue triage session so our CircleCI server is really busy. You can run meteor self-test and meteor test-packages on your local machine to see if the tests pass right away or wait 3 hours or so for this to hit Circle's queue.

@mitar
Copy link
Collaborator

@mitar mitar commented May 11, 2016

Somebody is not paying for more parallel builds at CircleCI. ;-)

@tmeasday
Copy link
Contributor

@tmeasday tmeasday commented May 11, 2016

Our tests are just like.... reallly slow. We have a lot of parallelism :/

@mitar
Copy link
Collaborator

@mitar mitar commented May 11, 2016

Do you use my runner from Blaze? It runs all the packages in parallel.

@tmeasday
Copy link
Contributor

@tmeasday tmeasday commented May 11, 2016

It's not the package tests, but the self tests which are the problem.

@zol
Copy link
Contributor

@zol zol commented May 11, 2016

Ok, one more thing to do @deanius - Can you please rebase this to be off of current devel? This branch is off an old commit that doesn't include the CI integration (hence CI is reporting no test commands were found). It should be as easy as (assuming you have a remote called upstream pointing here):

# optionally
git remote add upstream https://github.com/meteor/meteor

git fetch upstream
git rebase upstream/devel
git push --force
@deanius deanius force-pushed the deanius:computation-run-added-4514 branch to aa2fedc May 12, 2016
@deanius
Copy link
Author

@deanius deanius commented May 12, 2016

@zol - rebase and squash complete.

@zol zol merged commit 7a1fec0 into meteor:devel May 12, 2016
3 checks passed
3 checks passed
CLA Author has signed the Meteor CLA.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zol
Copy link
Contributor

@zol zol commented May 12, 2016

Thanks @deanius - merged!

@tmeasday
Copy link
Contributor

@tmeasday tmeasday commented May 12, 2016

Can we get a PR to vNEXT of meteor/docs for the docs part? Appreciated
On Thu, 12 May 2016 at 10:21 AM, Zoltan Olah notifications@github.com
wrote:

Thanks @deanius https://github.com/deanius - merged!


You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub
#4710 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants