Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Discussion for SSE extension syntax #618

Closed
benpate opened this issue Oct 14, 2021 · 12 comments
Closed

Discussion for SSE extension syntax #618

benpate opened this issue Oct 14, 2021 · 12 comments
Milestone

Comments

@benpate
Copy link
Collaborator

benpate commented Oct 14, 2021

We're moving SSE support into an extension (so it was decreed by @1cg). This thread is to collect suggestions/feedback on the syntax that we use in the new code. I'm just getting started now, so please let me know if I'm missing anything, or if there's something we should add. Now is the time :)

I think we should keep it as close to the existing syntax as possible, which should make it easier to migrate to the new code. Here's my first pass:

<div hx-ext="sse" sse-url="/my-first-event-source" sse-swap="message">
    INDIVIDUAL SUBSCRIPTION: I will be swapped whenever an unnamed message is received
</div>

<div hx-ext="sse" sse-url="/my-second-event-source" sse-swap="event1">
    INDIVIDUAL SUBSCRIPTION: I will be swapped whenever a message named "event1" is received
</div>

<div hx-ext="sse" sse-url="/my-third-event-source">
    SHARED SUBSCRIPTION:
    Nothing will happen to THIS div, but its children will listen to messages instead.

    <div hx-ext="sse" sse-swap="message">
        I will be swapped whenever an unnamed message is received
    </div>

    <div hx-ext="sse" sse-swap="event1">
        I will be swapped whenever a message named "event1" is received
    </div>

</div>
@benpate
Copy link
Collaborator Author

benpate commented Oct 14, 2021

SSE questions for @1cg, @dz4k, and the rest of the crew:

1) Publish additional htmx utilities? The existing SSE code (and probably WS code, too) uses a number of internal functions such as getClosest(), selectAndSwap(), and settleImmediately() that we'll almost certainly need to continue using. I don't see any way to get into the htmx internals from the outside. I'd recommend either:

  1. injecting a reference to the main htmx object into every extension.
  2. exposing more htmx "guts" to external code.
  3. something better?

2) Configuration Options? Currently, developers can configure SSE behavior by overriding the htmx.createEventSource() configuration setting. Is there an existing way to send configuration information to an extension? If not, we could either:

  1. make a way to pass data into an extension after it's been registered. For instance, we could publish the htmx.extensions Object so that developers could reach inside and fiddle with them.
  2. require developers call an initialization function to register the SSE extension -- e.g. calling initSSE(options) would register and initialize the extension.
  3. something better?

3) Existing hx-trigger events? The existing SSE code can also trigger artificial events that get picked up by hx-trigger. From the outside of htmx, I think the best thing to do would be to trigger a real event, probably based on the event source and message name, like "sse:<source_url>:<message_name>". We could also "name" the EventSource so that events look something like "sse:<source_name>:<message_name>" This would be a small change from the existing syntax, which can get away with sse:<message_name> because it's bound inside of the element containing the hx-sse tag. Any suggestions on keeping this consistent vs. changing the event names?

I'm sure more issues/questions will come up, and I'll try to post them here so that everyone can weight in.

@1cg
Copy link
Contributor

1cg commented Oct 15, 2021

I like injecting htmx (or a wrapper around its internals, maybe an extension API object?) into the extension.

@1cg
Copy link
Contributor

1cg commented Oct 15, 2021

for configuration options, I don't mind if extensions add their own to the standard object, so long as it is documented clearly

for events, I like triggering a real event, so long as it is backwards compatible

@benpate
Copy link
Collaborator Author

benpate commented Oct 15, 2021

I'll update my plans for each of these items accordingly. I like the addition of passing in an "API object" instead of all of htmx. I'll put together a minimal version of this that is needed for the SSE extension.

On events, we can just use the existing naming. The only drawback is that they'll be global now, so message names could collide if two SSE sources use the same names. This is probably unlikely, but you know someone is going to do this.

@benpate
Copy link
Collaborator Author

benpate commented Oct 23, 2021

I'm back into this project for a bit, and can now pass an API object into each extension (branch here). I'm now working to make the list of functions to pass in the API object. Here's my first question:

The current SSE code includes the following code to call additional extensions:

withExtensions(elt, function(extension){
    response = extension.transformResponse(response, null, elt);
});

I think this made sense before, but it feels pretty hanky to be writing an extension that then potentially forks out to other extensions. It seems like we could be enabling bad behavior. Here are two possible solutions/workarounds:

  1. Cut this code from the SSE extension, so that transformResponse is no longer available to SSE messages. (Is this even being used in the wild?)
  2. Move this code into a new, higher-level swap function that we make available to extensions via the api object.

EDIT: @1cg -- For now, I'm running an experiment with option 2. I've added a (temporary?) function to the api object called swap(elt, content) that just "does the right thing" with a set of content and an element. I'm sure this function is incomplete, but it is a copy/paste of the code that hx-sse was using, so it should at least work for this use case. If this works for you, I'd love to see this become a part of htmx itself -- it could even be used in a few other places where we swap in data, for instance in the htmx.ajax function.

@benpate
Copy link
Collaborator Author

benpate commented Oct 23, 2021

I'm also adding a new event htmx:beforeCleanupElement to the cleanupElement() function so that the SSE extension (and eventually WebSocket) knows it's safe to close an EventSource object. Please let me know if you see a better way to accomplish this.

@benpate
Copy link
Collaborator Author

benpate commented Oct 24, 2021

IMPORTANT QUESTION ABOUT EXTENSIONS for @1cg and @dz4k

I ran into a problem where hx-ext="sse" attributes were being overlooked in the document. I traced it back to findElementsToProcess() which looks for http verbs like hx-get and hx-post. As a part of this "extension" project, I've removed the query selectors that search for `hx-sse, but this means that the SSE extension was being skipped altogether.

I think this is because extensions normally enhance or modify the behaviors of the core features like hx-get and hx-post. But in the case of SSE, the extension needs to operate entirely on its own. They need to be handled by processNode just like the hx-sse tag was.

Does this make sense so far?

To keep this branch moving, I'm adding hx-ext into the list of attributes that findElementsToProcess searches for. But this is almost certainly the wrong path forward, and would probably break something if we leave it in for the long term.

I need your help to figure out a way forward that will not break existing extensions but will allow new extensions like SSE and WebSockets to be found and processed correctly.

@dz4k
Copy link
Collaborator

dz4k commented Oct 24, 2021

One possible approach is to let extensions provide their own findElementsToProcess() function, and have our function call them and union their results.

@1cg
Copy link
Contributor

1cg commented Oct 24, 2021

Adding hx-ext to the list of element to process seems extremely reasonable to me.

@benpate
Copy link
Collaborator Author

benpate commented Oct 24, 2021

Adding hx-ext to the list of element to process seems extremely reasonable to me.

Ok. Thank you, both of you. I'll continue from here and will try to test this with a few other extensions before the PR. This will need several other sets of eyes in it before it makes it into a release.

@benpate
Copy link
Collaborator Author

benpate commented Oct 26, 2021

GitHub had a meltdown and it broke my previous fork. I've posted all of my progress to a new fork/branch here: https://github.com/benpate/htmx/tree/pr-sse-extension.

It's coming along well, and nearly all of my tests are working great. I have to resolve one more issue with hx-settle and then I'll be looking to streamline the code. My current plan:

  1. Troubleshoot remaining issues and add retry code from How to reconnect to SSE (Server Sent Events) #522.
  2. Rough-in support for WebSockets, using this as a guide.
  3. Look for common features/problems between the two that need to be resolved.
  4. Submit one unified PR for both extensions.

Currently, the "private api for extensions" object works, but I'm not thrilled with it. It seems like a lot of overhead for a small benefit to rarely used code. Once I have completed WebSockets, I'm thinking it might be better to just ask to put the few methods I need into the public API instead. But, we can cross that bridge when we get there.

One last thing: progress is quick, but my time seems limited. When are you aiming to ship version 1.7?

@benpate
Copy link
Collaborator Author

benpate commented Oct 29, 2021

OK. I've just posted more changes to https://github.com/benpate/htmx/tree/pr-sse-extension

  1. New Extension architecture that passes a private variable to an init() function. This will require more testing from @1cg and @dz4k before it gets merged.
  2. Manual connection retries in case the browser barfs on us (address How to reconnect to SSE (Server Sent Events) #522)
  3. One small change from the original behavior: settle options such as settle delay are not working. If it's important to keep this feature then I'm going to need help to make it work correctly.

Please take a look and let me know how this looks to you, and how I can make it better.

@1cg 1cg added this to the 1.6.1 milestone Nov 2, 2021
@bigskysoftware bigskysoftware locked and limited conversation to collaborators Nov 22, 2021
@1cg 1cg closed this as completed Nov 22, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants