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

Expose bare minimum to add effects dynamically. #3460

Closed
wants to merge 1 commit into from

Conversation

kaste
Copy link
Contributor

@kaste kaste commented Feb 23, 2016

Adds methods to add and remove property effects. Basically:

  var fx = this.addPropertyEffect(propertyName, function(path, value, old) {});
  /// later
  this.removePropertyEffect(fx);

Reference issue #3456

Fixes #3420, fixes #1796, fixes #2131, fixes #2638, fixes #1778.

@samccone
Copy link
Contributor

samccone commented Mar 4, 2016

Hey @kaste this is super great and very exciting -- I for one have wanted this feature for quite a while, it opens up the door for all kinds of dynamic element creation while being able to take advantage of the built in data-binding behaviors of polymer! Thanks for the PR

@TimvdLippe
Copy link
Contributor

All issues that are fixed by this PR must be included in the PR description. Also they should all be prepended with Fixes. For more information see https://github.com/PolymerElements/ContributionGuide/blob/master/CONTRIBUTING.md#submitting-pull-requests

@kaste
Copy link
Contributor Author

kaste commented Mar 4, 2016

@TimvdLippe Updated the decription.

@fooloomanzoo
Copy link

I have tried the pull request with Array splices and can say it worked.

        var dynamicEl = document.createElement('child-element');

        this.$.main.appendChild(dynamicEl);

        this.addCustomEffect('user', function(path, value, oldValue) {
          // console.log(path, value, oldValue);
          if (value && value.indexSplices) {
            var index = value.indexSplices[0].index;
            var addedCount = value.indexSplices[0].addedCount;
            for (var i=index; i < index+addedCount; i++)
              dynamicEl.push("user", value.indexSplices[0].object[i]);
          }
        });

Appart from your nice, working solution, the array observation is just so unhandy, that you can loose joy and spoil the delight for yourself and there could be a nicer implementation for array-collections.

@kaste
Copy link
Contributor Author

kaste commented Mar 14, 2016

@fooloomanzii Isn't this what you want: ❓

    this.addCustomEffect('user', function(path, value, oldValue) {
      // console.log(path, value, oldValue);
      if (path === 'user.splices') {
        dynamicEl.notifySplices('user', value.indexSplices);
      }
    });

@fooloomanzoo
Copy link

@kaste I wasn't aware of notifySplices. Thank you, that makes a lot easier. Looking forward for your feature to be pulled

@devinivy
Copy link

devinivy commented Apr 2, 2016

👍 This is an exciting PR.

@kevinpschaaf
Copy link
Member

The general concept for this feature is good. We are doing explorations around a re-layering of Polymer that would expose the underlying machinery of the data system to end-users to enable better customization, and being able to add property effects to instances is definitely something we plan to include in that, so we're on the same wavelength here.

As far as adding this feature to the 1.0 codebase, the addCustomEffect / removeCustomEffect API is basically ok. However, I'd probably try to make these changes:

  • To avoid the need for code to re-create the accessor on the instance, I would try avoiding closing over the effects in the setter (in createAccessors) and instead reference them from the instance to simplify the code; this would need to involve performance testing that change as it is in a data hot-path, but the extra complexity of the current approach may not pay for any performance benefit
  • In removeCustomEffect, rather than going async (which we always try to avoid), I would instead copy the list & splice the effect (which is safe since _effectEffects closes over the original effects list); I'm not as much concerned about penalizing the remove operation with this cost, since if we were going to design the effects system to have good removal characteristics (which was not a goal in this design), we might consider changing everything to use a linked list (could be an option in 2.0)
  • Off the top of my head, I'm not keen on the disabled gambit; we consider depending on the order of effects running generally bad, and this code seems to imply that if a given custom effect is removed by an effect earlier in the list it should not run, but if it is removed by an effect later in the list it should run. This has a bad code smell to me, although it is hard to justify without studying the problem more.

@kaste I would say that if you're willing to consider & adjust based on the feedback above to go for it and we can take another look. As I said we are definitely planning to expose API for instance-time property effect management in the next major version where it will hopefully be a more fully-considered system, but that's still a little further down the road.

@kaste
Copy link
Contributor Author

kaste commented Apr 10, 2016

Currently _createAccessors is used by the Templatizer. So we rely on prestored effects here. (This is the current version in master. Your newer version that does not create accessors 9a4a8b8 is not merged yet.)

The side effect of having prestored effects here is that I need the async call. B/c there is no other option than mutating the propertyEffects, I cannot reassign a sliced effects-array. So there is that.

Regarding the disabled flag. I actually mimic the exact same behavior native DOM event listeners with their add/removeEventListener have. Only two rules here: Adding a new listener (effect) is not immediate, in the sense that this new handler will not be called before the next notification will be dispatched. Removing a listener though MUST be immediate.

The straightforward implementation for this is as follows::

    dispatchEffects() {
        var listeners = this.listeners.slice(); // !
        for (var listener of listeners) {
            if (listener.disabled) continue;
            listener.callback.call(...);
        }
    }

    addEffect(fx) {
        this.listeners.append(fx);
    }

    removeEffect(fx) {
        fx.disabled = true;
        this.listeners.remove(fx);
    }

This is described here https://dom.spec.whatwg.org/#events
I think I leave this PR as it is, and open a new one with a patched createAccessors.

@kaste
Copy link
Contributor Author

kaste commented Apr 10, 2016

#3573 has some of the suggested changes. Implementing a linked list is not among them ;-)

@kaste kaste force-pushed the bare-minimum-for-custom-effects branch from 0ee70ac to 658e6d8 Compare April 11, 2016 20:33
@kaste
Copy link
Contributor Author

kaste commented Apr 11, 2016

Renamed API to add/removePropertyEffect.

@@ -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!

@btelles
Copy link

btelles commented May 1, 2016

Anyone know when this PR is going to be merged and released? It really is a huge improvement. Thanks @kaste.

@kaste
Copy link
Contributor Author

kaste commented May 3, 2016

Note: B/c Templatizer makes some fu with propertyEffects I had to fight f9d64e1#diff-65d1593d7f79b51a64ac3cfc9465a1f4L330 to make instance property effects (this or #3573) work. (I actually used #3573 b/c it already has a better Templatizer but this Templatizer is unfortunately not in master.)

@kaste
Copy link
Contributor Author

kaste commented May 3, 2016

@btelles The coupling between Templatizer and the Polymer.Bind system, and my veto on removing the disabled flag, -> they need to meet.

@joryphillips
Copy link

I would greatly benefit from being able to do this! Any time estimates at this point? Thanks.

@kaste
Copy link
Contributor Author

kaste commented Aug 12, 2016

Ping.

This is now undecided for half a year. It adds value to the 1.x branch and is a non breaking change. You should prefer #3573 of course. On top that ‘linkPath‘ and ‘notifyPathUp‘ can be easily reimplemented as I have shown in other PRs, which will make Polymer faster for everyone.

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

Sorry for the very late response. Based on this PR and the numerous community requests, this API actually shipped in Polymer 2. You can try it out yourself in http://jsbin.com/toholosuhu/edit?html,console,output For a full documentation of the API you check out the docs: https://www.polymer-project.org/2.0/docs/api/mixins/Polymer.PropertyEffects#method-addPropertyEffect

@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