bigskysoftware / htmx

</> htmx - high power tools for HTML
https://htmx.org
Other
38.19k stars 1.3k forks source link

HX-Location feature: push url options (example of working code, tiny change) #2706

Open Wookiee81 opened 3 months ago

Wookiee81 commented 3 months ago

Currently HX-Location auto pushes the url to history (expected default behaviour).

if (hasHeader(xhr, /HX-Location:/i)) {
      saveCurrentPageToHistory()
      let redirectPath = xhr.getResponseHeader('HX-Location')
      /** @type {HtmxAjaxHelperContext&{path:string}} */
      var redirectSwapSpec
      if (redirectPath.indexOf('{') === 0) {
        redirectSwapSpec = parseJSON(redirectPath)
        // what's the best way to throw an error if the user didn't include this
        redirectPath = redirectSwapSpec.path
        delete redirectSwapSpec.path
      }
      ajaxHelper('get', redirectPath, redirectSwapSpec).then(function() {
        pushUrlIntoHistory(redirectPath)
      })
      return
    }

If possible could we gain some control over pushUrlIntoHistory(redirectPath) specifically gain the ability to set to 'false' or a custom location? An alternative would be to let HX-Replace-Url response header work with it; currently it does not seem to fire after this swap so does not work in this instance. HX-Trigger works with HX-Location (which is why I was using it) as I want to send custom trigger events with the redirect but I want to specify the url pushed or have none at all, this might not be intended behaviour (a bug). HX-Trigger does not work with standard Location response header or I would have just used that.

Sorry if this is a dumb request (there was a simple solution to my issue in Django). But it strikes me that this functionality could be useful to more people than just my edge case here. The HX-Location is described as doing a client side redirect without a full page reload. As such the push url seems to be something we might want more control over.

Disregard if I am just an Eedjit.

Wook.

Telroshan commented 3 months ago

I wonder if we should maybe simply reuse the existing history update events here as well? https://github.com/bigskysoftware/htmx/blob/89b9e2afa5fbc62ab7a22292e84f41d7da25447e/src/htmx.js#L4699C28-L4702

Though now that I look at it, it seems we can't abort the history update from those events

Wookiee81 commented 3 months ago

I wrote my solution and tested it. Fairly simple change. I can put in a pull request if anyone would be interested.

But I agree this is not very DRY as there is a HX-Push-Url and HX-Replace-Url header options... they just don't seem to work with HX-Location for some reason. Looking at the code we could run a check for them and defer to their conditions... but that might require a better understanding of the entire code base than I have. I just focused on this one section and changed it there.

if (hasHeader(xhr, /HX-Location:/i)) {
      saveCurrentPageToHistory()
      let redirectPath = xhr.getResponseHeader('HX-Location')
      var pushPath
      /** @type {HtmxAjaxHelperContext&{path:string}} */
      var redirectSwapSpec
      if (redirectPath.indexOf('{') === 0) {
        redirectSwapSpec = parseJSON(redirectPath)
        // what's the best way to throw an error if the user didn't include this
        redirectPath = redirectSwapSpec.path
        if (redirectSwapSpec.push !== 'false' ) {
          pushPath = redirectSwapSpec.push || redirectPath
          delete redirectSwapSpec.push
        }
        delete redirectSwapSpec.path
      }
      ajaxHelper('get', redirectPath, redirectSwapSpec).then(function() {
        if (pushPath !== undefined) { 
          pushUrlIntoHistory(pushPath)
        }
      })
      return
    }

All it does is add the potential for a "push" field, true by default (not needed), if false does not push, if a value push that value.

Can't set the value to "true" it will just push it to history. But its a simple filter to add if it needs it.

Don't really know how to write a test for this (I just put it up on my own website and and tried the options). I could probably fumble through it if needed.

(self taught programmer, lot I don't know).

Wook

Wookiee81 commented 3 months ago

Ok I will just polish this up, figure out tests, update docs and submit a pull.

It seems that most pull requests here are not waiting for things to be OKed in the issues so I will just follow suit.

Wook.

Wookiee81 commented 3 weeks ago

Sorry I just got pinged about finishing this up. The code works but I am having trouble writing the unit tests for it... I am not a programmer in any professional sense so this part has been stumping me.

Then I got some contract work which took up the last 3 months.

Is it ok to push this added feature without writing unit tests for it? It passes all the existing unit tests no problem.

Nathan Price feel free to submit yours instead if you like. I can't figure out the unit tests :D Nor Git really.

Wookiee81 commented 3 weeks ago

Hi Nathan,

Sure thing, please feel free to do yours. I cannot for the life of me figure out the unit testing to write my own. (self taught and this one is doing my head in)

The change is simple.

Just changing HX-Location to:

if (hasHeader(xhr, /HX-Location:/i)) {
  saveCurrentPageToHistory()
  let redirectPath = xhr.getResponseHeader('HX-Location')
  var pushPath
  /** @type {HtmxAjaxHelperContext&{path:string, push:string}} */
  var redirectSwapSpec
  if (redirectPath.indexOf('{') === 0) {
    redirectSwapSpec = parseJSON(redirectPath)
    // what's the best way to throw an error if the user didn't include this
    redirectPath = redirectSwapSpec.path
    if (redirectSwapSpec.push !== 'false') {
      pushPath = redirectSwapSpec.push || redirectPath
      delete redirectSwapSpec.push
    }
    delete redirectSwapSpec.path
  }
  ajaxHelper('get', redirectPath, redirectSwapSpec).then(function() {
    if (pushPath !== undefined) {
      pushUrlIntoHistory(pushPath)
    }
  })
  return
}

Works and passes all tests, just cant seem to figure out how to write my own 😄

Hope it helps.

Rob.


From: Nathan Price @.> Sent: Monday, 7 October 2024 6:26 PM To: bigskysoftware/htmx @.> Cc: Dr Robert McDowell @.>; Author @.> Subject: Re: [bigskysoftware/htmx] HX-Location feature: push url options (example of working code, tiny change) (Issue #2706)

Ok I will just polish this up, figure out tests, update docs and submit a pull.

It seems that most pull requests here are not waiting for things to be OKed in the issues so I will just follow suit.

Wook.

Hi, I looked at your fork and saw you stopped working on this. Do you think you're close? It's a feature my project would benefit from. I can put together my PR otherwise. I'm unsure how to maintain your commit history, so if you want me to finish it, I'll credit your work in my PR.

— Reply to this email directly, view it on GitHubhttps://github.com/bigskysoftware/htmx/issues/2706#issuecomment-2396128824, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AA67K6OTAVGOHAG7DYHGXYTZ2IZTXAVCNFSM6AAAAABKWAFYNCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJWGEZDQOBSGQ. You are receiving this because you authored the thread.Message ID: @.***>

gravityfargo commented 3 weeks ago

I deleted my comment at the last minute, but I see you're responding from email, so you still had it. Oof. Anyway, I read through the git documentation and kept your authored commits in my fork. I improved your logic; there was a more straightforward way to do it. I'm also held up on the tests but have written a working test. It turns out history is not enabled in the tests, which is why all the other tests related to history check HTML values and not the paths themselves. I'm inquiring in Discord about whether enabling that configuration option for my tests is allowed.

Wookiee81 commented 3 weeks ago

Oh man THAT explains a lot.

I bashed my head against that wall for so long I just put it on the infinite back burner. Glad you had a better way to do it. I ended up just working around it in my own project but this seemed to be a much cleaner way to go. Thanks for putting my name in there even though I am guessing my main contribution was just hitting that wall a few months prior.

Cheers and good luck.

Rob.


From: Nathan Price @.> Sent: Tuesday, 8 October 2024 11:12 AM To: bigskysoftware/htmx @.> Cc: Dr Robert McDowell @.>; Author @.> Subject: Re: [bigskysoftware/htmx] HX-Location feature: push url options (example of working code, tiny change) (Issue #2706)

I deleted my comment at the last minute, but I see you're responding from email, so you still had it. Oof. Anyway, I read through the git documentation and kept your authored commits in my fork. I improved your logic; there was a more straightforward way to do it. I'm also held up on the tests but have written a working test. It turns out history is not enabled in the tests, which is why all the other tests related to history check HTML values and not the paths themselves. I'm inquiring in Discord about whether enabling that configuration option for my tests is allowed.

— Reply to this email directly, view it on GitHubhttps://github.com/bigskysoftware/htmx/issues/2706#issuecomment-2398247627, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AA67K6PZE5LKD4LGYVJKFMDZ2MPNNAVCNFSM6AAAAABKWAFYNCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJYGI2DONRSG4. You are receiving this because you authored the thread.Message ID: @.***>