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

Bare minimum for custom effects (take 2) #3573

Closed
wants to merge 4 commits into from

Conversation

kaste
Copy link
Contributor

@kaste kaste commented Apr 10, 2016

Same as #3460. Removed prestored effects in createAccessors as suggested per #3460 (comment).

@kaste
Copy link
Contributor Author

kaste commented Apr 10, 2016

The last commit here is a minor refactoring in Templatizer that fits on top of your 59b5f24 which is a rebased version from #3440. (Of course my patch is also needed.)

Removes _pathEffector override completely.
Replaces _notifyPathUp override.
@kaste kaste force-pushed the bare-minimum-for-custom-effects2 branch from aa88fdb to f6b107d Compare April 11, 2016 20:42
@@ -274,10 +274,12 @@
var fx$ = this._propertyEffects && this._propertyEffects[model];
if (fx$) {
for (var i=0, fx; (i<fx$.length) && (fx=fx$[i]); i++) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to self: Must prestore length just like in accessors!

@@ -224,7 +228,7 @@
// queued events during configuration can theoretically lead to
// divergence of the passed value from the current value, but we
// really need to track down a specific case where this happens.
value = target[property];
value = target.__data__ ? target.__data__[property] : target[property];

Choose a reason for hiding this comment

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

I believe this reads a little clearer.

value = (target.__data__ || target)[property]

@tjsavage tjsavage added the 1.x label Sep 8, 2016
@TimvdLippe
Copy link
Contributor

Closing per #3460 (comment)

@TimvdLippe TimvdLippe closed this Aug 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants