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

Prefix custom event names #257

Merged
merged 7 commits into from
Nov 4, 2021
Merged

Conversation

alex-ketch
Copy link
Contributor

@alex-ketch alex-ketch commented Nov 3, 2021

This PR closes #253. It:

Because HTML attributes are case insensitive, browsers interpret any
uppercase characters as lowercase. This can cause problems across frameworks.
As such, for standardized usage we use lowercased kebab-style formatting for
custom events.
@nokome
Copy link
Member

nokome commented Nov 3, 2021

Regarding event naming conventions, I have been using the convention of type:id:verb to allow flexibility for subscriptions. For example, to subscribe to all events for all documents subscribe to "topic" document:*. To subscribe to all events for a specific document: document:do-f8JdSN8321hwkdfjde:*, and to a specific event for a specific document: document:do-f8JdSN8321hwkdfjde:patched. This is implemented in Rust and the Node.js bindings. For example, when you do document.subscribe() it is generating those topic strings in the background.

I think it's a useful convention, mostly because it reduces noise: subscribers only ever receive events for topics that they are subscribed to. But also because clients then do not have to check that ids match if they are only interested in changes for a particular document or node within a document.

Regardless of the change to kebab case, I think we should use the "subject-verb" convention ie type-id-verb, type-property-verb, or noun-verb. Also, I think that we should use past tense verbs where appropriate (i.e. when the event signals that something happened already, rather than should be done). For example, use changed or updated instead of set. Given that, we'd have these names:

  • stencila-set-all-code-visibility -> stencila-node-codevisibility-changed
  • stencila-collapse-all-code -> stencila-node-codevisibility-all (?)
  • stencila-set-editor-layout -> stencila-codeeditor-layout-changed
  • stencila-set-language -> stencila-codeeditor-language-changed

When using @Listen decorators we can't specify the id of the subject e.g.

@Listen('stencila-node-patched', { target: 'window' })

To get around this, I suggest that the web client subscribe to the server events using the id of the document as it currently does (alternatively listeners could be set up manually):

/**
 * Subscribe to a document topic
 */
export async function subscribe(
  client: Client,
  documentId: DocumentId,
  topic: DocumentTopic,
  handler: (event: DocumentEvent) => void
): Promise<Document> {
  client.on(`documents:${documentId}:${topic}`, handler)
  return client.call('documents.subscribe', {
    documentId,
    topic,
  }) as Promise<Document>
}

but that the receivePatch handler unpacks the received Patch and for each operation that applies to Web Component-ed nodes (it may have other operations to be applied to plain old HTML elements) dispatch a stencila-node-patched event (i.e. with the id not included) with a custom event that looks like

export interface NodePatched {
  nodeId: DocumentId
  patch: Patch
}

Currently the receivePatch function does something like this currently but needs some generalization and for the chosen naming convention to be used.

@alex-ketch
Copy link
Contributor Author

alex-ketch commented Nov 3, 2021

There are a couple of different points here that I'd like to untangle, mainly event names and behaviours in the stencila/web|client|rust|etc, and the behaviour as far as these WebComponents are concerned.

I think it's a useful convention

I agree, and I had also followed your example in the component listeners for the patch events.
As part of this PR though, I was debating whether to rename just the Designa/component related event names, or even the document patch related ones.
In the end, I thought that it would be better to avoid users having to remember two conventions and keep things the same across the board.

[...] because it reduces noise: subscribers only ever receive events for topics that they are subscribed to. But also because clients then do not have to check that ids match if they are only interested in changes for a particular document or node within a document.

This is where I think the context is different.
Unless I'm misunderstanding, it sounds like all open documents will still be receiving all messages, but since there won't be a listener attached it will be a no-op.
Whereas I was imagining a scenario where documents would initialize a subscription for the rendered document, and then the server would only send it relevant messages. Meaning the client wouldn't have to either receive or filter out messages for other documents.

When using @listen decorators we can't specify the id of the subject e.g.

@Listen('stencila-node-patched', { target: 'window' })

To get around this, I suggest that the web client subscribe to the server events using the id of the document as it currently does (alternatively listeners could be set up manually):

This gets to the second issue of ergonomics with DX.
Setting up listeners manually isn't too bad within Designa, however when used from other frameworks or projects attaching additional handlers becomes painful.
For example:

// MyComponent.tsx in pseudo code follows

// with kebab case
// ... some boilerplate
render () {
  return <stencila-article onStencila-documents-patch={e => console.log(e.detail.documentId)}></stencila-article>
}

// with colons, one possible solution
// ... some boilerplate
MyArticle = () => {
  const article = <stencila-article></stencila-article>
  article[`onStencila-document:patch:${topic}`] = (e) => console.log(e.detail)
}

render () {
  return <MyArticle></MyArticle>
}

Key being that we lose the ability to set the handler as props/attributes on the components.


Regardless of the change to kebab case, I think we should use the "subject-verb" convention ie type-id-verb, type-property-verb, or noun-verb.

For performance reasons it's better to use as few listeners as possible, so having a the subjects in there makes it difficult, and a little redundant as the emitted event contains the target.
Secondly it limits reusability of events, both from discoverability during development as as well as when trying to listen to the same global event across components (e.g. showing/hiding source code for both CodeChunks and CodeExpressions).

Also, I think that we should use past tense verbs where appropriate (i.e. when the event signals that something happened already, rather than should be done). For example, use changed or updated instead of set. Given that, we'd have these names:

This one I have a more of an opinion, and would prefer to stick to standard DOM Event convention e.g. (onLoad, onChange, dragStart, etc.). Also we have to keep in mind that these are not just past events, but also used when dispatching/triggering the desired listeners (e.g. document.dispatchEvent('stencila-set-editor-layout', {....}))

With that in mind though, I'll try and rename the events to include your feedback

@nokome
Copy link
Member

nokome commented Nov 3, 2021

What events the each page web client receives

This is not what is happening at present 👇🏽

Unless I'm misunderstanding, it sounds like all open documents will still be receiving all messages, but since there won't be a listener attached it will be a no-op.

This is what is happening at present 👇🏽

Whereas I was imagining a scenario where documents would initialize a subscription for the rendered document, and then the server would only send it relevant messages. Meaning the client wouldn't have to either receive or filter out messages for other documents.

☝🏽 That is what I meant by "subscribe to the server events using the id of the document as it currently does". In other words, each web client will only receive events for the topic, which includes the documentId that it subscribes to on this line:

client.on(`documents:${documentId}:${topic}`, handler)

The handler can then rename the event to whatever it wants when calling window.dispatchEvent.

Naming conventions

For performance reasons it's better to use as few listeners as possible, so having a the subjects in there makes it difficult, and a little redundant as the emitted event contains the target.
Secondly it limits reusability of events, both from discoverability during development as as well as when trying to listen to the same global event across components (e.g. showing/hiding source code for both CodeChunks and CodeExpressions).

I suspect you are taking a strict interpretation of "subject" (i.e. a unique identifier for an instance) and perhaps didn't look at my examples e.g. stencila-node-codevisibility-changed. I'm only suggesting to include id where it makes sense (probably never for browser custom events, but probably always for server generated events) and make things generic when appropriate e.g. codevisibility. It more using a consistent "subject-verb" order rather than "verb-subject" order which I'm advocating for. What the "subjects" and "verbs" are is less of a concern.

BREAKING CHANGE: Event names have changed, please refer to the component
README for updated versions.
BREAKING CHANGE: Please use `programmingLanguage` prop,
or `programming-language` HTML attribute instead.
@alex-ketch alex-ketch merged commit d895ca6 into master Nov 4, 2021
@alex-ketch alex-ketch deleted the 253-prefix-custom-event-names branch November 4, 2021 18:53
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.

Prefix custom events with stencila: string to avoid future namespace clashes
2 participants