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

[RFC] Global events naming convention #131

Closed
3 tasks
skjnldsv opened this issue Jul 17, 2020 · 24 comments
Closed
3 tasks

[RFC] Global events naming convention #131

skjnldsv opened this issue Jul 17, 2020 · 24 comments
Labels
enhancement New feature or request help wanted Extra attention is needed question Further information is requested

Comments

@skjnldsv
Copy link
Contributor

skjnldsv commented Jul 17, 2020

Intro

Welcome!

Since I find it quite awesome that we can use global events, I think we should aim for a nice convention.
Let's list an example of potential events we could see across Nextcloud... 🤔

Thoughts

  • A file have been updated
  • An app have been enabled/disabled
  • A contact have been deleted
  • A file have been uploaded
  • A header menu have been opened
  • The sidebar have been toggled
  • ...

Goals

I think it would be nice to have some kind of modifier syntax.
I really dislike underscores, so I would favour dashes.

Proposal

  • files:file.update
  • contacts:contact.delete
  • nextcloud:app.disable
  • nextcloud:unified-search.search

Sub-proposal

I would love to be able to watch for partial event names.
Like file:update:12345 and file:update would both trigger if the file 13245 is updated?

Questions

  • Shall we enforce the dashes and throw errors
  • Shall we strongly suggest the syntax event:modifier:data (e.g. file:update:12345)
    • If so, the header-menu-unified-search:open should most likely be header-menu:open:unified-search ?

Thanks!
cc @georgehrke @korelstar @ma12-co @juliushaertl @ChristophWurst @raimund-schluessler @danxuliu @rullzer

@ChristophWurst
Copy link
Contributor

@ChristophWurst
Copy link
Contributor

A more high level question: do we want to prefix the event names with app IDs, if applicable? Like if more apps start using events the chances of a conflict arise if the events are named too generic, like logout or whatever. Should it be mail:logout instead if the event is meant to be used in Mail only?

Of course that does not apply to global stuff that is possibly emitted by the Nextcloud server.

@skjnldsv
Copy link
Contributor Author

skjnldsv commented Jul 17, 2020

do we want to prefix the event names with app IDs, if applicable?

I like the idea, and thought about it, but will be a nightmare when the id is something like files_sharing. Then we'd have events like files_sharing:create or whatever?
I'm more willing to do things like share:create straight away?

@skjnldsv skjnldsv added enhancement New feature or request help wanted Extra attention is needed question Further information is requested labels Jul 17, 2020
@ChristophWurst
Copy link
Contributor

I'm more willing to do things like share:create straight away?

But that calls for conflicts.

Then we'd have events like files_sharing:create or whatever?

That sounds actually fine to me? It's very clear what app this belongs to.

@skjnldsv
Copy link
Contributor Author

Okay, so what about enforcing dashes with a nice str_replace on the library itself so everyone uses the same? :D

@ChristophWurst
Copy link
Contributor

Could you elaborate on your strong feelings against underscores? 🙊

@skjnldsv
Copy link
Contributor Author

Could you elaborate on your strong feelings against underscores?

it reminds me of lodash xD
No, it's just mixing both of them I dislike 🤷
I feel like it's messy for a naming convention ?

@ChristophWurst
Copy link
Contributor

If we want to scope things with an app ID then we should allow the _s. IMO it's not worth to forbid this sign just because of cosmetic preferences. We could use https://en.wikipedia.org/wiki/Snake_case but I'm sure you won't like it 😉

@skjnldsv
Copy link
Contributor Author

Okay then, let's move on! We go for appID every time? Or only for non-core ones?

What about the modifier syntax? files_sharing:create ? And the partial event trigger?

I would love to be able to watch for partial event names.
Like file:update:12345 and file:update would both trigger if the file 13245 is updated?

@ChristophWurst
Copy link
Contributor

Okay then, let's move on! We go for appID every time? Or only for non-core ones?

Prefix for app events, no prefix for core ones?

What about the modifier syntax?

I don't dislike it, but it would be quite different to the naming scheme with have for php events. Like if you create a file in php we'd use a FileCreated and with the modifier syntax we'd have file:create, right? The more streamlined alternative would be file-created or similar, depending on how we join multi word event names.

And the partial event trigger?

Sounds interesting but I do not have a real preference on that yet. Guess it depends on how much we use this.

@skjnldsv
Copy link
Contributor Author

To be fair I'm fine to go for any solution. But I feel we should really start to ensure this is done properly :)

@skjnldsv
Copy link
Contributor Author

Like if you create a file in php we'd use a FileCreated and with the modifier syntax we'd have file:create, right? The more streamlined alternative would be file-created or similar, depending on how we join multi word event names.

What made me thought of this was the vue event syntax which I kinda like :)
But I'm also fine with events like files-create contacts-delete mail-send.. etc 🤔

@ChristophWurst
Copy link
Contributor

But I'm also fine with events like files-create contacts-delete mail-send.. etc thinking

Though it should describe an event, not the action, hence use the past tense like file-created contact-deleted and mail-sent

@skjnldsv
Copy link
Contributor Author

So, any idea where we should document this? Shall we enforce it? i'm fine doing an lowercase on it 😉

@ChristophWurst
Copy link
Contributor

I'd say it should got into our dev docs because that is also where we'll add any global events emitted by the server.

And I would keep it simple and not transform the names (app prefix) too much.

@skjnldsv
Copy link
Contributor Author

I'd say it should got into our dev docs because that is also where we'll add any global events emitted by the server.

We could have an automated docs maybe?
That scans the Nextcloud org?

@ChristophWurst
Copy link
Contributor

I doubt this would work reliably. Also we want to be careful what we document as official and what should remain internal.

@skjnldsv
Copy link
Contributor Author

I doubt this would work reliably. Also we want to be careful what we document as official and what should remain internal.

If there is a comment describing the event, we list it? Otherwise e don't?
Worth a try?

@ChristophWurst
Copy link
Contributor

How would you do this? Scan all repos for supposedly emitted events? I don't get it

@skjnldsv
Copy link
Contributor Author

Nah, ignore this, it seems too complicated. I was thinking an external service like we do for translations for example

@ChristophWurst
Copy link
Contributor

https://stackoverflow.com/a/61641681/2239067 for the names

https://github.com/airbnb/javascript#events for pushing raw values vs an object (for easier modification later on)

@skjnldsv
Copy link
Contributor Author

skjnldsv commented Aug 31, 2020

Call feedback:

  • We enforce the kebab-case event naming
  • For apps events, please prefix your events: files:deleted
  • For global nextcloud events, we'll use nextcloud: as the prefix; nextcloud:unified-search.search nextcloud:unified-search.reset
  • We strongly advise against using events-to-trigger methods.
    • We will create a OCP api to allow apps to register interfaces and call methods from there. This will allow to have feedback on the execution
  • We will not create a dedicated emitTyped event bus method
  • All enforcing will be on the documentation level. We will not enforce on the runtime.
  • Global events will be documented on the dev docs. If your app have dedicated methods than other might want to listen to, this would also be the place (example, files:created)

@ChristophWurst since you're our @nextcloud/packages expert, how hard would it be to have a package for this? And how would it looks like? Wanna to a RFC?

What did we say about the pass tense though? 🙈

@raimund-schluessler

This comment has been minimized.

@nickvergessen
Copy link
Contributor

this is okay, but e.g. there is a global event emit('toggle-navigation'which you can use to open/close/toggle the sidebar. This is abuse and should have an API instead. there would then be an event to listen when this was done like there is currently emit('navigation-toggled'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants