search

naja-js / naja

Modern AJAX library for Nette Framework
https://naja.js.org
MIT License
109 stars 16 forks source link

fix: history disable #356

Closed lubomirblazekcz closed 2 years ago

lubomirblazekcz commented 2 years ago

@jiripudil history is currently not fully disabled when history: false is set. This should fix it.

History in naja is not friendly with https://swup.js.org/ currently, that's the main reason we need this disabled. That first replaceState on dom ready kind confuses swup. Because when you click back button in browser, swup thinks that page was not loaded with swup, so it does nothing and page is not loaded.

I am not sure of the reason why there has to be a replaceState on first load in the first load. Shouldn't this be only when naja request is made?

jiripudil commented 2 years ago

Hi, thanks, and please accept my apologies for the late response.

The initial replaceState() is there mainly so that SnippetCache can store a snapshot of the snippets in the history state (but any userland extension can also add its own data to the state). I'm worried your change could break in cases when history is initially disabled but later enabled for specific requests (if it even makes sense to do that?)

Shouldn't this be only when naja request is made?

I really like this suggestion though. HistoryHandler could do the setup only as soon as a request with non-false history mode is dispatched through Naja. I'll need to think it through more carefully, but it seems like it could be the way.

For the sake of completeness, you can hook onto the HistoryHandler's buildState event to add data to the state, e.g. for swup. This is the desired way for other scripts to integrate with Naja's history mechanism. But this is irrelevant in your case, I understand that's inconvenient when you want the history integration disabled entirely.

lubomirblazekcz commented 2 years ago

Hi @jiripudil, thanks for response :)

We would even use history integration, but currently it's causing us bugs with Swup, so we had to disable it on our projects completely. Not sure how to do this properly though, swup adds source property to each popState https://swup.js.org/options#skip-popstate-handling maybe that could help a bit?

https://github.com/naja-js/naja/blob/64bc1b17b093d9e8d82a175fedd8bfdb9316b0ba/src/core/HistoryHandler.ts#L123 adding source: 'naja' here, this way swup knows which is which.

But there is still problem with that first setup, if that could be changed only as soon as a request is made, that would fix the issue overall I think.

jiripudil commented 2 years ago

Hi, I've followed up on the idea in #362 and it seems to be working nicely. I've even added the source: 'naja' to the history state :) thanks for the suggestions!

I've released these changes in 2.4.0, feel free to open a new issue if you encounter any problems with it.

lubomirblazekcz commented 2 years ago

Had the chance to test this finally, and it works great with swup now, thanks!