-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
allow hx-headers to access event
#3462
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
base: dev
Are you sure you want to change the base?
Conversation
this includes the fake/custom event "load" which previously issueAjaxRequest was not aware of, from the callback set in processVerbs
|
Why have you added that load event? It seems unrelated to the rest of the PR. Would it be appropriate as a separate PR? You have added a parameter to |
It's related because the code example will fail if Will add the annotations, thanks |
| * @param {Element} elt | ||
| * @param {Element} target | ||
| * @param {string} prompt | ||
| * @param {string=} prompt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this because prompt is optional as shown in if (prompt !== undefined)
@MichaelWest22 Does this look good to you? |
|
https://jsfiddle.net/Latent22/xfb124jq/17/ The window.event is already going to this hx-headers js eval just fine and the example you give already works out of the box right now when i tested it in this jsfiddle. I can't reproduce the hx-trigger load thing that is causing you to need to create a dummy load event. When i test it in my fiddle I just get DOMContentLoaded as the triggering event which seems fine. can you give some examples showing why the custom 'load' event might be needed? I can reproduce the issue in the very edge case of a delayed request because then it is going to be in a setTimeout and not part of the triggering event but this seems very much an edge case. I think the proper place to create the 'hx-trigger-event' custom header is in a htmx:configRequest hx-on or eventListener or htmx extension. The htmx.configRequest does not need inline JS eval in an attribute and has access to the evt.detail.triggeringEvent even in a delayed situation and this is the ideal place to in the htmx event flow to configure custom request properties. document.body.addEventListener('htmx:configRequest', function(evt) {
evt.detail.headers['hx-configRequest-event'] = evt.detail.triggeringEvent?.type || 'load'
})htmx.defineExtension('trigger-event-header', {
onEvent: function (name, evt) {
if (name === "htmx:configRequest") {
evt.detail.headers['hx-trigger-event'] =
evt.detail.triggeringEvent?.type || 'load';
}
}
});Note that the only limitation of the configRequest event is it reports the real event and not window.event which will be undefined in the load situation so you have to adjust for this if needed. |
|
it works because
Quoting scrhartley PR,
the I'm trying to craft an example in jsfiddle that does something like this, where it shows the failure <main>
<div hx-get="/fragment" hx-trigger="load, click" hx-headers='js:{"hx-trigger-event": event.type}'
hx-target="closest main" hx-swap="beforeend">
Request a fragment
</div>
</main>where <div hx-get="/whatever" hx-trigger="load, click" hx-headers='js:{"hx-trigger-event": event.type}'>
Request a second fragment on load
</div>but it will fail on the second request because |
|
here is the example I crafted (like said above) that doesn't work |
|
OK, so in @scratchmex's example, the initial |
|
hmmm could not get that playcode.io to do a real partial.html fetch for me to see it failing sorry. created a jsfiddle one with sinon to mock the requests https://jsfiddle.net/Latent22/y5erwxuq/10/ This one throws a "Uncaught TypeError: Cannot read properties of undefined (reading 'type')" error because event can be null here so it currently needs a event?.type instead right now which i think would be fixed maybe with your change but it would need testing I think. I think the load event is already being created manually one layer up and if we had to fix this then this should be passed down into loadImmediately() function. I'm just not 100% sure if this is really a bug we should fix or not even if we were to fix the window.event -> real event part for the delay edge case which is kind of an extension of the #3196 fix. to me the event object should be best effort and it would be expected that in edge cases like load or htmx.ajax() usage the event object may not exist. So users should ideally be doing something like : <div hx-get="/fragment" hx-trigger="load, click" hx-headers='js:{"hx-trigger-event": event?.type || "load"}'
hx-target="#response">
Request a fragment
</div>so that all the edge cases like htmx.ajax() won't break things https://jsfiddle.net/Latent22/y5erwxuq/13/ here is the working example with this change. |
|
The ideal fix for the load event would be something like this: dev...MichaelWest22:htmx:load-trigger There is so much manual logic to do the load immediately function and htmx already has all the logic to do this same thing already with once and listen to a real event. But we can't use the real 'load' event as this is an actual browser supported event which is why it was probably done the hacky way it is right now to bypass events and just call a manual handler and re-implement the logic by hand. But if we were able to fire a different event name like htmx:trigger:load that is unique then it is much safer and you could now optionally prevent default on this custom event if you wanted to. This version fixes the no event issue in my fiddle but it doesn't fix the delay case. But htmx:configRequest method works well regardless. |
|
Nice improvement. I agree the "load" terrain is a bit muddy because you have
so actually thanks for setting up the site and your enhanced version michael. about making the delay work with your changes, just pass the event from I think your approach fixes more technical debt and make it more consistent by using streamlined functions
|
|
yeah I thought you wouldn't be able to use the browsers built in 'load' event as this is triggered by some element types like img but this seems to work fine in my quick testing as long as you don't bubble the manual load event. This simplifies things a little bit. tried updating my test branch above with this change |
|
I added your changes Michael and tests |
Co-authored-by: MichaelWest22 <[email protected]>
|
This implementation does introduce a change of behavior. <div hx-get="/example" hx-trigger="load" hx-on:load="console.log('loaded')">
Get Some HTML, Including a custom header indicating which event triggered the request
</div>I tried solving this with |
|
Is that not desired? Honestly looking at it feels intuitive that it should log, isn't it? |
|
It might be surprising since all other events bubble and so if someone moves the listener up the DOM it'll stop working and then they'd wonder why. If we decide it is desired then I guess we'd want a test for it. |
|
that's a good point. considering that the standard load event is triggered (only) on the window and does not bubble either,
I think we can define the behavior similar in which only the "htmx node" triggers that event and does not bubble either we can change the behavior to be bubbleable, in the future if it's needed, and will be safe to not be confused with the standard event because
Will be nice to add that remark in the documentation but I don't know which section, probably |
Yeah this is a change but the load event was never expected or fired before so there is no one using hx-on:load on a loaded element right now with a working solution. So this change just adds a new ability to listen for this event but it only applies to the load element itself. I don't think we can make the load event bubble like normal htmx events becuase then it could trigger the window load event listeners which would be unexpected and wierd. we want to keep this load event private just for the element so it can be used just by htmx. I don't think it would be good to document or encourage users to make use of this event explicitly. The load event also fires on img and iframe tags as well and this kind of load event is local to the elt only which is where I got the idea to make this load event do the same behaviour. I also tested doing an htmx load trigger on a img tag and this fires the load event twice with this custom change applied but it only triggered the htmx action once as expected so won't cause any problems. |
|
There are some other elements it fires on: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/load_event
I didn't realize the event doesn't bubble (unusual). I can't say I'm a fan of the event firing twice as that's a leaky abstraction. |
|
Yeah htmx:trigger:load could make more sense. I was just trying out 'load' to see if we could get away with not having to alter the trigger name like this and keep it cleaner. Good to try these things so you can tease out how it actually works. One issue is if we use htmx:trigger:load then you have to pick between a manual non bubbling event so it only fires on this element or fire a proper full htmx triggered event like normal which will also bubble and a load trigger on an element that loads in children with their own load trigger will bubble up to the parent again but it won't cause problems as the .once stops it triggering twice. |
|
The implementation of Does not send an ajax request when clicked (change of behavior): |
Yeah good catch. I doubt this would ever happen in reality but will not be handled as it was before. would have to move to htmx:trigger:load and make this a non bubbling event so it can only ever fire one time by design. someone could manually re-trigger the load by hand but I think that is not an issue. One other potential issue with the old 'load' version we tried is that if somehow firstInitCompleted is not false it would fall though and create a default addEventListener and this would then possibly respond to browser based load events in error. But one other edge case that is not handled the same maybe is multiple load triggers in the same hx-trigger. "load, load" will make it fire two times but this is not really an issue. but "load[shouldLoad1()], load[shouldLoad2()]" with two conditions would possible be valid and change a little as it will fire two htmx:trigger:load events and the first one if its true it would trigger two load events maybe. |
Description
When using
hx-valsandhx-varsyou can referenceeventas shown in the docs for hx-vals. That is not the case withhx-headers. This PRs allowhx-headersto accesseventin the js eval.This includes the fake/custom event "load" which previously
issueAjaxRequestwas not aware of, from the callback set inprocessVerbsThe PR is similar to what #3196 did
I added a documentation example of why this might be useful, to identify the trigger event name, feature was requested 2 years ago in reddit actually
Corresponding issue: #3461
Testing
I have patched my current project with this changes and the example added in the documentation works.
added these tests
Checklist
masterfor website changes,devforsource changes)
approved via an issue
npm run test) and verified that it succeeded