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

Implement core events for common actions #288

Closed
wants to merge 13 commits into from
Closed

Implement core events for common actions #288

wants to merge 13 commits into from

Conversation

ryangjchandler
Copy link
Contributor

@ryangjchandler ryangjchandler commented Mar 18, 2020

Here is my initial PR for implementing core events for common actions in Alpine. It closes #283 . There's room for improvement here still, but this is the initial PR to get some feedback and suggestions for some more events, etc.

These are the events that are getting fired currently.

  • alpine:loaded - fired at document level after Alpine has initialised all components.

  • alpine:mutated - fired at component level on the parent element when a piece of data is mutated

  • alpine:[key]-mutated - fired at component level on the parent element when the key data has been mutated

  • alpine:updated - fired at component level on the parent element when the component has been updated / refreshed.

  • alpine:transition-start - fired at transitioning element level but will bubble up so event listener can be attached to parent component too, will fire when the transition starts

  • alpine:transition-end - fired at transitioning element level but will bubble up so event listener can be attached to parent component too, will fire when the transition ends

To-do:

  • Add tests for events
  • Add documentation for events

cc/ @calebporzio @SimoTod @HugoDF

@HugoDF
Copy link
Contributor

HugoDF commented Mar 18, 2020

Just thinking out loud here, is there a going to be a noticeable overhead in emitting events on every mutation?

@ryangjchandler
Copy link
Contributor Author

ryangjchandler commented Mar 18, 2020

Just thinking out loud here, is there a going to be a noticeable overhead in emitting events on every mutation?

From my testing, the overhead is minimal. Of course, the overhead will grow with the number of event listeners that need to run on each mutation / update, but that is really down to the developer to manage. Still a very good point though and might be worth adding to the docs as a warning.

It does take the gzipped bundle size over 7kb, but I think that there are a few other places in the code that we can tidy up too that will help with that.

P.s. ignore all of these commits, I'm going to squash them :)

@ahmedkandel
Copy link
Contributor

I think alpine:${key}-mutated is repeating what $watch do, Also if we will have the key and value from alpine:mutated then alpine:${key}-mutated may not be needed.

A typo note it closes #283

It closes #288 .

@ahmedkandel
Copy link
Contributor

Another addition could be:
alpine:initialized - fired at component level on the parent element when the component has been initialized.

@ryangjchandler
Copy link
Contributor Author

I think alpine:${key}-mutated is repeating what $watch do, Also if we will have the key and value from alpine:mutated then alpine:${key}-mutated may not be needed.

A typo note it closes #283

It closes #288 .

I somewhat agree with you. The alpine:[key]-mutated event does replicate the $watch functionality, but it does also mean that your x-init directive doesn't get clogged up if you want to observe / watch multiple pieces of data.

It will also remove the need for using conditionals in your alpine:mutated listeners, much cleaner code.

The alpine:initialized event could work, but we would need a better name since that gives the impression that Alpine itself has initialized without knowing the event context / target.

@ahmedkandel
Copy link
Contributor

I do like the idea of having events 👍 even if it may seem similar to $watch for alpine:mutated and $nextTick for alpine:updated. As stated before events are more expressive and can be listened to from outside of Alpine. But alpine:mutated already do the same thing as alpine:${key}-mutated ?

alpine:initialized may be renamed to alpine:firstupdate or may be a better idea to name the events with the context somthing like:

alpine:loaded , alpine-property:mutated , alpine-component:initialized , alpine-component:updated , alpine-transition:start , alpine-transition:end

@ryangjchandler
Copy link
Contributor Author

I do like the idea of having events 👍 even if it may seem similar to $watch for alpine:mutated and $nextTick for alpine:updated. As stated before events are more expressive and can be listened to from outside of Alpine. But alpine:mutated already do the same thing as alpine:${key}-mutated ?

alpine:initialized may be renamed to alpine:firstupdate or may be a better idea to name the events with the context somthing like:

alpine:loaded , alpine-property:mutated , alpine-component:initialized , alpine-component:updated , alpine-transition:start , alpine-transition:end

@ryangjchandler
Copy link
Contributor Author

Accidentally clicked close 🤦🏻‍♂️.

Yeah, the mutated events both do the same thing and fire at the same time, but remember that the alpine:mutated event will fire for all data changes, even if your listener / callback is not interested in them, you'll need some conditional logic to only do something. With the alpine:[key]-mutated event, you can be certain that your callback will only run when [key] has been mutated.

If the events were to be more specific, I would prefer to place the context after alpine:, for example alpine:component-initialized or alpine:component-updated.

@ahmedkandel
Copy link
Contributor

If the events were to be more specific, I would prefer to place the context after alpine:, for example alpine:component-initialized or alpine:component-updated.

Better 👍

Regarding alpine:[key]-mutated if you want to observe/watch multiple pieces of data, You will have to create multiple listeners while with alpine:mutated one listener including switch(event.data.key) will make it simpler. Anyway it is up to you 😉

@ryangjchandler
Copy link
Contributor Author

I'd say it's better to give the developer both options. Switch statements can get ugly too, I don't like them.

@ryangjchandler
Copy link
Contributor Author

Gonna close this PR and open a more formal one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Component lifecycle events
3 participants