WebReflection / hyperHTML

A Fast & Light Virtual DOM Alternative
ISC License
3.07k stars 112 forks source link

[Question] Custom Event support ? #71

Closed bigopon closed 7 years ago

bigopon commented 7 years ago

First of all, these lines are amazing L214-L226 👍 💯

But is there a way you can refactor is a bit to support custom event ?

WebReflection commented 7 years ago

Those functions are pre-resolved and the least checks I do the better.

Events are recognized via 'on' prefix but also must be valid.

Example: onchange makes sense for a select but it won't do anything on a div.

Accordingly , I'm not sure how would you define custom events or what would you expect.

bigopon commented 7 years ago

It's my usage of hyperHTML, as I'm trying to create an abstract layer for inter-components communication, by leveraging `events. So it would be very helpful if I have the ability to do:

render`
  <div class="lists-wrapper" onselectperson="${someListener}">
    <!-- List 1 (People) here -->
    <!-- List 2 (Products) here -->
  </div>
`

I think it would be better than rolling my own version of Parent-Child programmatic events

Or maybe it would be appreciated to have some guideline for better design.

WebReflection commented 7 years ago

What do you expect to happen? What kind of events are you expecting to trigger there?

Will you later on dispatch a selectperson event?

WebReflection commented 7 years ago

Please note I'm not trying to avoid changes I actually believe sooner or later this will be needed regardless.

I am trying to figure out what's the best approach and how users would expect/use such feature.

What I am thinking right now is the following:

Latter point will make changing events slower, and I'm still not sure I'd go for it regardless ... hacking now, if you have any hint/answer please share, thanks

bigopon commented 7 years ago

From People list wrapper element, i will:

// in the people list component
peopleListWrapperEl.dispatchEvent(new CustomEvent('selectperson', { detail: personData, bubbles: true });

// in the two lists container components
handleEvent(e) {
  if (e.type === 'selectperson') {
    // ... adjust the products list
  }
}

But there is way for me to avoid using DOM events for communication, I just need to make better models (too much code to comment). Or I can do the following for example (What i can only think of so far)

class ShoppingManager {
  people: Person[]
  displayProductsFor(p: Person) {
    // ...
  }
  handleClick(e) {
    this.displayProductsFor(getSomePersonFromClickEvent(e));
  }
}

class Person {}
class Product {}

But doing it that way quite breaks component encapsulation as the parent is handling the click deep inside its children. So I'm not sure. How are you using hyperHTML to do complex stuff then ?

WebReflection commented 7 years ago

I haven't bumped the version yet or published anything, but I'd like your opinion about latest master version.

You can basically now add any custom event you want.

The behavior, is basically the exact same you'd expect from native events such onclick or others, let me show you an example.

const render = hyperHTML.wire({some:'object'});
const div = render`
  <div class="lists-wrapper" onselectperson="${console.log}">
    <!-- List 1 (People) here -->
    <!-- List 2 (Products) here -->
  </div>
`;

// div, as wire with a single node, refers to the `lists-wrapper`
// you can now dispatch directly bubbling, and canceling events
// the same as an input.click(); would do
div.selectperson({optional: 'detail'});

what do you think? Following the rules.

Rules

Last point means that onwhateverReally="${listener}" will assign a node.whateverReally(optionalDetails) functionality, same as onclick has a counter node.click() part and onblur has a counter node.blur() method.

I'm not sure I went too far with the latest point but I feel like it's the thing everyone would expect (or has dreamed about for years).

Any sincere thought on this current proposal in master would be more than appreciated.

Thank you.

sourcegr commented 7 years ago

Thanks to both of you :)

I believe that the feature request was very very clever.

And I also believe that the solution proposed is excellent; both on the implementation part but also on the possibilities it gives, which are endless. Although we should probably test it in more detail, it looks like a game changer for the communication of components

my sincere admiration :)

WebReflection commented 7 years ago

Shall I minor bump and publish? 😄

sourcegr commented 7 years ago

well, actually I need to find some use cases for this now! 🤔

WebReflection commented 7 years ago

It works well with Custom Elements and containers, where nested children can dispatch custom events as they like ... there your use case? :smile:

WebReflection commented 7 years ago

Custom Elements a part I'm waiting for an opinion from @bigopon so that if everyone is happy (I am) we should be good to go with this endless possibilities update :wink:

joshgillies commented 7 years ago

@WebReflection really nice work on this one! 👌

bigopon commented 7 years ago

When a node listens to a custom event, hyperHTML will also give it the ability to dispatch that event by calling the function with corresponding name. I think you expected me to do this instead of your example code:

var peopleListElement;

var shoppingElement = render`
  <div class="lists-wrapper">
    ${peopleListElement = renderPeopleList`
      <div class="people-list-wrapper" onselectperson=${{ handleEvent() { /* Change products.... */ } }}>
        <!-- some click events will happen inside here -->
      </div>
    `}
    ${renderProductList`
      <div class="product-wrapper">...</div>
    `}
  </div>
`

// The handle click will looks like this:
handleClick() {
  peopleListElement.selectperson(peopleStore.get(e.target.id));
}

This doesn't look like what i was asking for in this issue and I'm not sure if I wanna go this far either. I was just hoping for simple custom event hook. But it looks like it's forcing me to have a dispatcher, and also break my component scope (Feel hard to explain by text)

TL;DR: New commit makes any node listening to any events both listener and dispatcher, I'm not sure about dispatcher part but I do want the listener part. The reason is because it's DOM event, so I may put the listener on a parent element. (Like a list of components, we often put the listener on the parent.)

WebReflection commented 7 years ago

To be honest, as soon as I've written "not sure" and "too far" I've realized indeed making nodes dispatcher of their own custom events wasn't a good idea.

I'll happily remove that extra step and I'll keep the rest.

Please note you don't have to use objects with a handleEvent method, you can simply pass a callback as listener too.

Last question

What do you think about always using addEventListener for events?

It's slower on reassignment but it also ensures only the node owner can drop that listener so it preserves safety .

jfrazzano commented 7 years ago

Question:

What is the downside of taking say all your inputs on a form, running a map.reduce in a get and assigning

Object.defineProperties(context, “inputs”,{ “enumerable”:true, configurable:true, get:()=>{ Array.from(this.form.querySelectorAll(‘input.animatedinput’)).map(a=>a!==null&&a.nodeName===“INPUT”?a.onmousedown=a.ontouchstart=a.onfocus=a.onkeypress=a.onkeydown=a.onpointercapture=e=>true?(this.doshit(e),a):””).reduce(complexobj, b,i, array)=>{ do stuff… },{handler:somobjecthandler, []))

this is my current approach,

previously i split the events in code that handles WAI ARIA combo box navigation:

here is the first piece starting at the array from

this.inputs=Array.from( this.fcform.querySelectorAll("input[type='text']") ).map((a,i)=>{ a.value=this.valueObject[a.name]; a.onfocus= e =>{ this.input===undefined?this.input=e.target:(this.input.parentElement.classList.remove("expand"),this.input=e.target); e.target.addEventListener("input",this.bindInputs); this.resetDropdownDisplay(); }; a.onblur= e =>{ e.target.removeEventListener("input", this.bindInputs);

                        };
        a.onkeydown= e =>{
    switch(e.key){
                case 'Enter':

                e.preventDefault();
            case 'Tab':
                this.listchildren=this.doFilter(this.input.value);
                    Array.isArray(this.listchildren)&&this.listchildren.length===0
                        ?(this.dropdownData(this.input.value),this.resetDropdownDisplay())
                    :this.listchildren[0].dataset.value===this.input.value?(this.listchildren[0].querySelector("input").checked=true, this.listchildren[0].focus()):this.listchildren[0].focus();
            if(e.key==='Tab'&& e.key!=='Shift+Tab'){this.listchildren[0].blur();window.setTimeout(()=>{this.inputs[Math.floor(i+1)].focus();},450)}
                    break;
                case 'ArrowDown':
                case 'ArrowUp':
                            e.preventDefault();
                this.listchildren=Array.isArray(this.listchildren)&&this.listchildren.length===0?Array.from(this.ddmholder.querySelectorAll("fc-valueitem")):this.listchildren;
            console.log(this.listchildren, this.ddmholder,this.listchildren[this.ddmIndex]);
                    this.listchildren[this.ddmIndex].focus();
        break;
        case 'Tab': 
            this.tabClick=true;
        break;
        case "Backspace":
            this.resetDropdownDisplay();
            break;
        case "Escape":
            this.removeDdmholder();
            break;

                    }
            }
                        return a;
        })

this is preceded by binding to a data object, data map, local storage, idk, firebase , then to a registration object I am using as my observer.

I believe a reflect could do it all but haven’t proven it yet. But that seems its purpose. If its a really bad idea, sorry. I just found it works, is fast, and can manage, bind, and keep the this for hundreds of inputs in forms used from user inputs to calendars, clocks, todos, etc.

Again… you know best. I just don’t know how to accomplish the same just with add event listener,

On Jul 24, 2017, at 9:55 PM, Andrea Giammarchi notifications@github.com wrote:

To be honest, as soon as I've written "not sure" and "too far" I've realized indeed making nodes dispatcher of their own custom events wasn't a good idea.

I'll happily remove that extra step and I'll keep the rest.

Please note you don't have to use objects with a handleEvent method, you can simply pass a callback as listener too.

Last question

What do you think about always using addEventListener for events?

It's slower on reassignment but it also ensures only the node owner can drop that listener so it preserves safety .

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/WebReflection/hyperHTML/issues/71#issuecomment-317605734, or mute the thread https://github.com/notifications/unsubscribe-auth/AHxnvjjuSjpsFwynNQucInj69_MJTUXDks5sRUsjgaJpZM4OhHYI.

WebReflection commented 7 years ago

Please let's not go out of scope of this request.

I won't answer to other unrelated topics to keep this thread as clear as possible.

Thanks for your understanding

jfrazzano commented 7 years ago

On Jul 24, 2017, at 10:30 PM, Andrea Giammarchi notifications@github.com wrote: sure sorry;

like i said, you know best. I just had issues with scoping, using other option. my fault though. Thank you. And sorry.

Please let's not go out of scope of this request.

I won't answer to other unrelated topics to keep this thread as clear as possible.

Thanks for your understanding

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/WebReflection/hyperHTML/issues/71#issuecomment-317610644, or mute the thread https://github.com/notifications/unsubscribe-auth/AHxnvsQdYAzTNWTlqhtVTB-LTKLM6Bunks5sRVMwgaJpZM4OhHYI.

bigopon commented 7 years ago

For me, it is the expected behavior. I would leave node.onclick = someFn to user so they can do their dirty stuff if needed. Competing with lib users here is too ... unpredictable.

For performance, I think you know about performance a lot more than I do.

For capturing, is there any way ? probably will rarely be used, but it's maybe good mental exercise to think about it 😄 The reason i'm saying it's not necessary is because people will always find a component framework leveraging hyperHTML, instead of creating everything their own, even when it's simple with hyperHTML. And with framework, you rarely touch the element directly, thus there little need for this.

WebReflection commented 7 years ago

Semantically speaking, you assign events as if it's DOM Level 0 , where capturing is never possible.

I think for now I'll just publish custom events as it is, removing the dispatching part.

Will try to do it before this flight (I'm checking in now 😅)

jfrazzano commented 7 years ago

On Jul 24, 2017, at 10:57 PM, bigopon notifications@github.com wrote:

For me, it is the expected behavior. I would leave node.onclick = someFn to user so they can do their dirty stuff if needed. Competing with lib users here is too ... unpredictable.

For performance, I think you know about performance a lot more than I do.

For capturing, is there any way ? probably will rarely be used, but it's maybe good mental exercise to think about it 😄 The reason i'm saying it's not necessary is because people will always find a component framework leveraging hyperHTML, instead of creating everything their own, even when it's simple with hyperHTML. And with framework, you rarely touch the element directly, thus there little need for this.

100% right— — You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/WebReflection/hyperHTML/issues/71#issuecomment-317614376, or mute the thread https://github.com/notifications/unsubscribe-auth/AHxnvhUQVb7tnoYKxE6MEshBpyZcGxgUks5sRVmQgaJpZM4OhHYI.

jfrazzano commented 7 years ago

Good Luck, Andrea. You have done really great work. Its kinda awesome.

Thank you and have a good trip.

Sincerely,

Jason

On Jul 24, 2017, at 11:01 PM, Andrea Giammarchi notifications@github.com wrote:

Semantically speaking, you assign events as if it's DOM Level 0 , where capturing is never possible.

I think for now I'll just publish custom events as it is, removing the dispatching part.

Will try to do it before this flight (I'm checking in now 😅)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/WebReflection/hyperHTML/issues/71#issuecomment-317614971, or mute the thread https://github.com/notifications/unsubscribe-auth/AHxnvt7DP34873DlOMqpAkid8PvYWpK4ks5sRVqTgaJpZM4OhHYI.

WebReflection commented 7 years ago

v0.16 just landed with custom events enabled :tada: