The Wayback Machine - https://web.archive.org/web/20170406214516/https://github.com/vuejs/vue/pull/4652
Skip to content

Necessary changes to observer for full Tracker compatibility #4652

Open
wants to merge 8 commits into
from

Projects

None yet

6 participants

@mitar
mitar commented Jan 5, 2017 edited

(This is all open for debate and I understand that it is previously undiscussed pull request, but it is easier to debate it with concrete code changes available.)

I am working on integration of Vue with Meteor and I would like that Vue could be a drop-in replacement for Blaze. That is almost possible, because architecture is very similar. Vue is also using reactive dependency tracking, observer, while Meteor/Blaze is using Tracker. Now, there is a lot of code written for Meteor which is using Tracker and it would be great if it could just work with Vue. And in fact it is doable. I managed to create a full 100% compatible Tracker implementation on top of Vue's observer. But there are some changes necessary to observer's implementation and some internals have to be exposed.

This pull request is initial version of changes I had to do to get this first version of Tracker implemented, to get discussion rolling. I would really like to see if this or something like this could get merged into Vue. From my perspective changes do not really change how observer is working, and all existing tests are still passing.

A list of changes:

  • added support for after flush callbacks
  • adding a way to force flushing
  • fixed exception recovery for pushTarget/popTarget
  • correctly cleanup queue if there is an exception from getter
  • exposing some internals (probably too much, API could be cleaner there)

Requesting comments, feedback, suggestions. This is work in progress. I will add tests in the future, after we finalize that this is OK to do and everything else.

Example app. Notice how I can use Meteor ReactiveVar inside computed value directly.

@mitar mitar referenced this pull request in Akryum/vue-meteor-tracker Jan 5, 2017
Open

Integration between Vue reactivity and Tracker reactivity #3

@defcc
Member
defcc commented Jan 5, 2017

Thanks @mitar , you need to follow the contribution guide when opening a PR. Please refer: https://github.com/vuejs/vue/blob/dev/.github/CONTRIBUTING.md#pull-request-guidelines

@mitar
mitar commented Jan 5, 2017

@defcc: I thought I did. Which part from the guide I am missing? I will add tests later for new features. I will fix failing tests now. Is there anything else?

@mitar
mitar commented Jan 5, 2017

Hm, tests are now failing at "should get updated with model when in focus" which seems unrelated to my changes? Flaky tests?

@mitar mitar referenced this pull request in meteor/blaze Jan 5, 2017
Open

Vue as Blaze 2.0 #131

@defcc
Member
defcc commented Jan 5, 2017

Thanks @mitar , I opened a PR to fix that error.

@mitar
mitar commented Jan 5, 2017

Tests are passing.

@yyx990803
Collaborator

Hey, I think this is an interesting approach, but here's why I think it's unlikely to get merged: these changes increases the public API surface and will impact how we can make changes in Vue in the future.

By exposing the internals, it creates a liability for Vue to keep these APIs intact in order to stick to semver (otherwise it would break anything that relies on them, e.g. your Tracker wrapper). However, the scheduler and the reactivity system really is considered internal, and I intend to keep them internal so that we can iterate and make non-trivial changes to them without affecting the public API. There are a few experiments going on currently (e.g. using ES6 Proxy for change detection, slicing scheduler workload with requestIdleCallback) that all may cause changes to how the scheduler/Watcher/Dep system works, but if these internal implementation details become part of the public API, it places a serious constraint on how we can iterate on things in the future.

I do see the benefits here in that it allows the two reactivity systems to integrate more seamlessly, however I'm not sure whether it justifies adding the extra liability.

For now, a fork seems like the best short-term solution to me. In the long run, I will probably refactor Vue's reactivity system so that it can be exposed in a way that is less coupled to the internal details.

@mitar
mitar commented Jan 5, 2017 edited

@yyx990803, thank you for your answer.

I do understand your position, but I also think that there are some alternatives available here.

I am not sure what is your position of using _ prefixed symbols. But this could be one way to expose those internals in a way to communicate that those symbols are internal and could break at any point and using them requires that consumer makes sure things work and things are integrated correctly. For example, from our side we could simply exactly require specific version of Vue and upgrade it in Meteor package only after a new version is tested. But at the same time, some other developer can try and force a new version much easier than forking our fork and changing things there to get this new version.

Another approach would be to provide a very simple API to register an "observer" module with which one can override default observer code (Dep, Watcher, and queueWatcher). Only the API of those would be fixed, but internal implementation would be left to the module to implement.

@Akryum
Member
Akryum commented Jan 6, 2017

Exposing internals with _ while considering them not part of the public API seems fine to me. And the Meteor integration could pin the version of Vue.

@aadamsx
aadamsx commented Jan 19, 2017

@yyx990803, have you made a final decision on this? :)

We in the Meteor/Blaze community need this change in order to move over to your framework! There's a lot of developers over there waiting excitedly for this development!

Can't we compromise on this by using the "_" to expose just the few internals we need? We understand if you decided to make breaking changes to this portion of the API we're on our own. Heck, shut down the said APIs if there is a breaking change and we'll be responsible for updating said APIs and brining them back online once everything passes.

I believe the effort will expose a entire community to your UI framework, possibly increasing the mindshare and popularity!

@mitar
mitar commented Jan 19, 2017 edited

I love to see the enthusiasm of Meteor community for this. :-) Thanks.

Just to be clear, exposing symbols with _ is not enough, there are also some changes needed the code as shown in this pull request:

  • added support for after flush callbacks
  • adding a way to force flushing

(BTW, I will add tests for all those changes if we decide to merge this pull request.)

An alternative approach, of providing a way to register a custom implementation of Dep, Watcher, queueWatcher would allow us to simple completely swap the implementation, but of course the downside is that this common code would not be maintained (and especially coordinated!) together, which is a downside. But on the other hand it would not require from you to maintain this part of codebase.

I would also like to point out that some things should probably be merged in any case. I found out that current code is very bad at recovering from any exception, leaving the whole system in an undefined state. You can only reload to recover. This is why I added things like:

  • fixed exception recovery for pushTarget/popTarget
  • correctly cleanup queue if there is an exception from getter

Meteor Tracker tests are pretty hard on testing those edge cases and this is how I found them.

@jamiter
jamiter commented Jan 24, 2017 edited

If this will be possible, in whatever form, I dare say that Vue will become the default used framework with Meteor. At least for everyone still on Blaze. This would be the most sophisticated and incremental way to start using Vue. Good for both communities!

I started rendering some parts of our app with React. Although React is a great framework, it feels like a mismatch with Meteor. Or tracker, to be more precise.

I truly hope this interoperability will see the light of day!

@yyx990803 yyx990803 added the pending label Feb 12, 2017
@mitar
mitar commented Feb 28, 2017

Resolved merge conflicts with current dev branch.

@aadamsx
aadamsx commented Mar 1, 2017 edited

@yyx990803, you added a 'pending' label to this PR. Sorry, but what does this mean for us waiting on this change to integrate Vue into our existing Meteor projects? Do you plan to merge this PR?

Just in case: Blaze has been broken out from Meteor and is now a stand alone project. @mitar is the primary owner/maintainer for Blaze (no longer MDG). He is trying to use Vue as the foundation of a Blaze 2 that will serve as the "default" UI for Meteor (the alternative is React). But without this PR, it will be an impossible task.

@yyx990803, please allow the Meteor community to further migrate towards Vue by accepting this PR and helping @mitar where needed.

Thank you for a super great framework!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can't perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.
Morty Proxy This is a proxified and sanitized view of the page, visit original site.