Skip to content
This repository has been archived by the owner on Jul 30, 2018. It is now read-only.

Add Matches meta provider #638

Merged
merged 2 commits into from
Sep 7, 2017
Merged

Conversation

kitsonk
Copy link
Member

@kitsonk kitsonk commented Aug 18, 2017

Type: feature

The following has been addressed in the PR:

  • There is a related issue
  • All code matches the style guide
  • Unit or Functional tests are included in the PR

Description:

This PR provides a meta provider that can determine if a particular event's target is the rendered version of a particular virtual DOM's key property.

This is indented to allow widgets to "filter" events that may have bubbled up or to allow a widget to do a level of event delegation with the DOM without having to know specifically about its rendered DOM.

@kitsonk kitsonk requested review from agubler and matt-gadd August 18, 2017 14:52
Copy link
Member

@agubler agubler left a comment

Choose a reason for hiding this comment

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

Did you mean to turn all the tabs to spaces in the readme?

Copy link
Member

@agubler agubler left a comment

Choose a reason for hiding this comment

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

Added some thoughts, also would be great if we actually implemented some kind of synthetic events which we could add the key of the target node (and other things) to so that users wouldn't need to use Meta that actually accepts a DOM node as an argument.

* @param event The event object
*/
public get(key: string, event: Event): boolean {
this.requireNode(key);
Copy link
Member

Choose a reason for hiding this comment

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

Adding the key as a required node is a bit confusing to me. Do you think that when used in an event handler, if the node1 has not been rendered that it should 1) cause an immediate re-render or 2) throw an error if the node doesn't still doesn't exist in the next render? of the back of a user action?

myEventHandler(event: Event) {
    if (this.meta(Matches).get('node1', event)) {
        // do something 
    }
    else if (this.meta(Matches).get('node2', event)) {
        // do something         
    }
    else if (this.meta(Matches).get('node3', event)) {
        // do something 
    }
    else if (this.meta(Matches).get('node4', event)) {
        // do something         
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I see the point.

  1. cause an immediate re-render

This wouldn't cause an event to re-fire, so really it doesn't do anything.

  1. throw an error if the node doesn't still doesn't exist in the next render?

Throwing an error when the node doesn't exist feels like the only real way to handle this, it is the only way to properly inform a mistake was made.

Copy link
Member

Choose a reason for hiding this comment

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

Okay what if the node1 one does exist and the successful user action causes the node to not be included (correctly due to the user action) in the next render, it would throw an error.... The seems a bit confusing to me (but it is the same for Dimensions too)

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, if the meta was in a render function, returning a dummy value instead of an error makes sense because it maybe part of a state/lifecycle where throwing isn't the right thing... here though, where this provider will ever be called inside an even listener, there is nothing that can be done... invalidating isn't a logical, so the assumption is that trying to match a key that doesn't exist is a terminal error and a throw is appropriate.

Copy link
Member

Choose a reason for hiding this comment

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

@kitsonk it's quite difficult to explain the scenario that I think will be unexpected as a user, I put together an example repo where errors are thrown because user actions cause nodes to be removed - in a way that I wouldn't think are expected... https://github.com/agubler/dojo2-matches-meta

Needs your branch of course :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yes... I see, yeah, I guess it can't throw then, it needs to simply return false. We can't get into a situation of trying to re-run a listener because a node wasn't found, just in case a re-render might make it appear. It needs to clearly just fail gracefully. 😖 It does highlight that we might need some sort of diagnostics for meta down the road to expose what sort of nodes are being requested to identify unintentional failures.

document.body.removeChild(div);
},

'calling before node'() {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what this is testing from the description.

rAF.reset();
}

registerSuite({
Copy link
Member

Choose a reason for hiding this comment

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

Do you need a test that for a test for a key that doesn't exist in the tree?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not to get 100% coverage on the code, as an invalid key is the same logic as a not-yet created key.

public get(key: string, event: Event): boolean {
this.requireNode(key);
const node = this.nodes.get(key);
return node ? node === event.target : false;
Copy link
Member

Choose a reason for hiding this comment

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

isn't return node === event.target good enough?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I condensed down some logic that was two different statements and realise the folly of the logic now.

@kitsonk
Copy link
Member Author

kitsonk commented Aug 22, 2017

also would be great if we actually implemented some kind of synthetic events which we could add the key of the target node (and other things) to so that users wouldn't need to use Meta that actually accepts a DOM node as an argument.

@matt-gadd and I chatted about that when discussing this PR. The challenge is that it would mean re-writing every DOM event going to every listener and dealing with the challenges of bubbling and cancelling and modelling and not only resolving target to a key, if it exists, but also currentTarget and likely other standard properties that reference back to these "synthetic events".

Ultimately, that is part of why we talked about passing event instead of event.target so that if we ever did come up with a replacement for DOM events (which I think is a complicated and potentially hugely performance impactful change) we would need to break any code as it is the provider that is introspecting the event.

I think the best we can do is give some abstraction at this point.

@agubler
Copy link
Member

agubler commented Aug 25, 2017

@matt-gadd and I chatted about that when discussing this PR.

It could be helpful to have the conversation(s) on the issues/pull requests even if they end up being just a replay of the ones held offline 😄

@kitsonk
Copy link
Member Author

kitsonk commented Aug 25, 2017

It could be helpful to have the conversation(s) on the issues/pull requests even if they end up being just a replay of the ones held offline 😄

Hard to predict that every semi-related conversation (like re-writing the entire DOM event system) would be germane to what PR. 😁

@kitsonk kitsonk merged commit 35f4d05 into dojo:master Sep 7, 2017
@dylans dylans added this to the 2017.08 milestone Sep 7, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants