Closed qwtel closed 1 year ago
Sorry took me a moment to get back to this.
I went through it and decoupled the dom types so that it would work in environments that don't have dom types available.
I am going to migrate the code out of get-navigation and make it so that all the functionality is added when needed, which can include the actual inclusion of the file, allowing me to then ignore these dom types in runtimes that don't need them, say deno. Its possible with imports to just to import a no-op file instead.
Heres the branch: https://github.com/virtualstate/navigation/tree/feature/polyfill
https://github.com/virtualstate/navigation/actions/runs/4267901334/jobs/7429965585
The above has bun/deno/node tests running
There are some browser tests that I see aren't running atm though.
I think its important we try and targt keeping everything together so we can mark off that we have collectively achieved the WPT/Web Platform Tests available for navigation
I reverted the readme and top level changes like workflows to retain them, as they are meant to be able to run in any repo as well. They are free to run GitHub public repos.
Which brings up a good point, being able to apply the polyfill needs to be highly selective so that we can still test in these WPT/Playwright/Browser environments very specifically and not change things up in the global state unless we really want to.
I'm going to work with this code a bit and get something working that also retains state for bun/deno/node, allowing for some wider testing of the state itself.
I have now got the code you have provided into its own function that doesn't use any global state, and have retained your intention I believe, with most of the code still the same.
Where the getState function was defined, I changed it to "getStateFromWindowHistory" which assumes that you're using the global window.history.state value
All of the static keys you had defined, I had added, in, and also allowed a bit more.
const {
persist: PERSIST_ENTRIES,
persistState: PERSIST_ENTRIES_STATE,
history: HISTORY_INTEGRATION,
limit: PERSIST_ENTRIES_LIMIT,
patch: PATCH_HISTORY,
interceptEvents: INTERCEPT_EVENTS,
window: givenWindow,
navigation: givenNavigation
} = options
This now allows testing of the code in node/deno/bun using something like
const navigation = new Navigation(),
history = new NavigationHistory({ navigation }),
window = new EventTarget(),
document = new EventTarget();
window.document = document;
window.history = history;
// Gets a new evented polyfill using the window, doesn't event based on the global window or history
const newNavigation = getPolyfill({ window })
// Applies an already existing polyfill to the global scope
applyPolyfill({ navigation })
// Applies a new polyfill to the global scope, based on the history in the window
applyPolyfill({ window })
// Applies a new polyfill to the global scope, based on the history in the global window
applyPolyfill()
// Imports all the needed polyfill code, and calls applyPolyfill with the navigation returned from the default getNavigation
await import("@virtualstate/navigation/polyfill");
Where no options are given, there is a frozen DEFAULT_POLYFILL_OPTIONS
defined
export const DEFAULT_POLYFILL_OPTIONS: NavigationPolyfillOptions = Object.freeze({
persist: true,
persistState: true,
history: true,
limit: 50,
patch: true,
interceptEvents: true
})
Another add I have is that if there is no window.history, we can still polyfill history as well, as I made a reverse polyfill for history, built on top of navigation
So if you do...
const window: EventTarget & { history?: History } = new EventTarget();
applyPolyfill({ window });
const { history } = window;
history
should work as expected.
Then because you have updated the core entries getState
function, this is what is used by the history polyfill, so it would be all tied in together.
Okay I think I have enough of a change so far, I do need to add some tests directly of the polyfill doing its thing, but I will for now leave it for you to review and if you're happy with it maybe merge into your branch and try it out?
I moved the getState
pattern so that the entire function can be customised through passing a function through to the constructor of navigation
const navigation = new Navigation({
getState({ key }) {
const maybe = sessionStorage.getItem(this.id);
if (!maybe) return undefined; // Or can return null here
return JSON.parse(maybe);
}
})
It is used in the place that the polyfill code was needed to initiate state:
getState<ST extends S>(): ST;
getState(): S;
getState(): unknown {
let state = this.#state;
if (!state) {
state = this.#state = this[NavigationGetState]();
}
This then allows for another direction that can be externally configured
new Navigation({
getState({ key }) {
const maybe = sessionStorage.getItem(this.id);
if (!maybe) return undefined; // Or can return null here
return JSON.parse(maybe);
},
setState(entry) {
const state = entry.getState();
if (state) {
sessionStorage.setItem(entry.key, JSON.stringify(state));
} else {
sessionStorage.removeItem(entry.key)
}
}
})
You could write your own setState
using currententrychange
, but this here is just first class support for it, and makes sense seeing it alongside getState
Theres another part to this too... what happens when we dispose entries from the list (I saw this was in yourcurrententrychange
event handler too)
https://github.com/WICG/navigation-api#notifications-on-entry-disposal
So it might warrant one more function. Which would be called the same time as navigation.currentEntry.addEventListener("dispose", fn)
would be called. So, I would call it, disposeState
new Navigation({
getState({ key }) {
const maybe = sessionStorage.getItem(key);
if (!maybe) return undefined; // Or can return null here
return JSON.parse(maybe);
},
setState(entry) {
const state = entry.getState();
if (state) {
sessionStorage.setItem(entry.key, JSON.stringify(state));
} else {
sessionStorage.removeItem(entry.key);
}
},
disposeState({ key }) {
sessionStorage.removeItem(entry.key);
}
})
Well this is something fun, while testing the polyfill, in Safari/Webkit, I get this:
{status: rejected, reason: SecurityError: Attempt to use history.pushState() more than 100 times per 30 seconds}
I think this might be related to some of my changes, but, its something we might have to throttle, or maybe I have to turn off when doing things like unit tests.
Its interesting that this even exists, I wonder how it's a security concern.
https://github.com/virtualstate/navigation/blob/feature/polyfill/src/tests/navigation.scope.ts
I have some assertions here that just confirm we have added the navigation to the global scope. We could detect that we are dealing with the polyfill here and then add some more tests specifically around the polyfill integrations.
Well this is something fun, while testing the polyfill, in Safari/Webkit, I get this:
{status: rejected, reason: SecurityError: Attempt to use history.pushState() more than 100 times per 30 seconds}
I think this might be related to some of my changes, but, its something we might have to throttle, or maybe I have to turn off when doing things like unit tests.
Its interesting that this even exists, I wonder how it's a security concern.
Ah yes, I come across that one ever so often. It's possible to trigger it manually, just navigation history via a shortkey. Doesn't have much practical implication, but probably bad for testing.
Closing in favour of issue discussion, the commits have been merged already into a working branch (feature/polyfill)
Related #9