WebReflection / heresy

React-like Custom Elements via V1 API builtin extends.
https://medium.com/@WebReflection/any-holy-grail-for-web-components-c3d4973f3f3f
ISC License
276 stars 16 forks source link

Best practice for triggering event methods #37

Closed dethe closed 3 years ago

dethe commented 3 years ago

More a question than a bug.

if I have

class MyComponent extends HTMLElement{
  static get name(){ return "MyComponent"; }
  static get tagName(){ return "my-component";}
  onclick(){ alert("I've been clicked!"); }
}
define(MyComponent);

What is the recommended way to subscribe the component to its events? I see the eventListener part of your article on the event pattern, but not the part that detects the onXXX methods and autosubscribes them. Is adding listeners in init() the recommended way, or is it just to each their own way? Or have I missed some opt-in feature like returning a static list of events I'm interested in?

Thanks!

WebReflection commented 3 years ago

it's already subscribed, every listener that starts with on gets registered and initialized, so once you click that, the component is the context and you can alert this.tagName

dethe commented 3 years ago

OK, that's the first thing I tried. I've just grabbed the latest version and tried again and the handler still isn't getting called. I tried with one of the CodePen examples and didn't get the click event. I've tested in Firefox and Chrome, on Windows and Mac. I was able to get it to work by inheriting from a Handler class inspired by your article (the code in the article didn't work because it uses this and .prototype in a static method):


  constructor() {
    super();
    console.log('events: %o', this.events);
    this.events.forEach(evt => this.addEventListener(evt, this));
  }

  // lazy static list definition
  get events() {
    let proto = Object.getPrototypeOf(this);
    return proto._events || Object.defineProperty(
      proto, '_events',
      {value: Object.getOwnPropertyNames(proto)
                    .filter(type => /^on/.test(type))
                    .filter(type => !['onconnected', 'ondisconnected', 'onattributechange', 'oninit'].includes(type))
                    .map(type => type.slice(2))}
    )._events;
  }
dethe commented 3 years ago

So, I'm still not sure what I'm doing wrong, but I couldn't find anything in the code base which does the above. If I'm mistaken, I'm happy to be corrected, otherwise if this is supposed to work but doesn't (i.e., if I'm not the only one this is not working for) I'm happy to put the above in a PR.

WebReflection commented 3 years ago

it is possible events are getting attached without the on prefix ... can you please try the following instead?

class MyComponent extends HTMLElement{
  static get name(){ return "MyComponent"; }
  static get tagName(){ return "my-component";}
  click(){ alert("I've been clicked!"); }
}
define(MyComponent);

if this works, I think this might be indeed a valid bug, if it doesn't, I think something went wrong with latest pushes 🤔

WebReflection commented 3 years ago

this is supposed to work

yes, as indeed examples uses onthings around, so I'm not fully sure why it's not working, but looking at the code, something is wrong, so like I've said, this might be indeed a very valid bug somehow I didn't catch before.

dethe commented 3 years ago

So, I'm still not sure what I'm doing wrong, but I couldn't find anything in the code base which does the above. If I'm mistaken, I'm happy to be corrected, otherwise if this is supposed to work but doesn't (i.e., if I'm not the only one this is not working for) I'm happy to put the above in a PR.

dethe commented 3 years ago

Yes, I had already tried adding click(){} vs. onclick(){} without success. Glad to know this is a feature which is not working and not just me losing my mind 😄

dethe commented 3 years ago

Also, my code above for Handler is not working (although I could have sworn it worked yesterday). It is returning an empty list now instead of ['onclick']. I've tried for too long to get it working and then realized I'm fixing it in the wrong place and instead of getting the Handler workaround functioning, I should fix it in heresy. I'll try installing rollup, etc. and see if I can get it patched.

WebReflection commented 3 years ago

I'll have a look, as something doesn't seem right, but I've been working on different abstractions recently, so it might be just something I don't remember, as tests are green, and people used heresy a lot already so ... not sure what's going on

dethe commented 3 years ago

Thanks, I don't have sed on my work machine, so it's difficult to build things here.

WebReflection commented 3 years ago

quick update, this demo uses event handlers and it works without any issue: https://webcomponents.dev/edit/6JTOoIOI1dwSyUq0Xvo7

I am still not sure what's the issue here, as heresy always worked like that, but indeed that demo is about defining listeners to its children, while if you want to define listeners to the component itself, I guess the best approach is to do that oninit

class MyComponent extends HTMLElement{
  static get name(){ return "MyComponent"; }
  static get tagName(){ return "my-component";}
  oninit() { this.addEventListener('click', this); }
  onclick(){ alert("I've been clicked!"); }
}
define(MyComponent);

Listeners added through the template literal will be owned by listeners providers, but the component itself can also use its own prototype to define listeners either on connected, removing these on disconnected, or on init, if these are nt meant to be removed.

This should answer your question, so if that's the case, please close this bug, thanks!

dethe commented 3 years ago

OK, that answers my question. I just need to be more explicit with the handlers than I thought. Not a problem. Thanks for looking into it and clarifying.

WebReflection commented 3 years ago

Handlers are more common for inner elements, and if added automatically would trigger twice each time.

You can, however, create an portable oninit method that does that dance for you, if it’s your commo use case.