-
Notifications
You must be signed in to change notification settings - Fork 10
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
POC Monkey-patch history pushState and replaceState to listen to location changes #455
base: main
Are you sure you want to change the base?
POC Monkey-patch history pushState and replaceState to listen to location changes #455
Conversation
68a5305
to
2f3f8f0
Compare
@humitos this just does the monkey-patching for now (haven't added any listeners yet). Do we want to do a test-build with this, to see if it correctly picks up the "page changes", before we move on to the next step? |
2f3f8f0
to
7aee62e
Compare
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 did a small review and left a few questions.
* Setup events firing on history pushState and replaceState | ||
* | ||
* This is needed when addons are used in SPA. A lot of addons rely | ||
* on the current URL. However in the SPA, the pages are not reloaded, so | ||
* the addons never get notified of the changes in the URL. | ||
* | ||
* While History API does have `popstate` event, the only way to listen to | ||
* changes via pushState and replaceState is using monkey-patching, which is | ||
* what this function does. (See https://stackoverflow.com/a/4585031) | ||
* It will fire a "pushState" or "replaceState" event, depending on which method was called. |
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.
* Setup events firing on history pushState and replaceState | |
* | |
* This is needed when addons are used in SPA. A lot of addons rely | |
* on the current URL. However in the SPA, the pages are not reloaded, so | |
* the addons never get notified of the changes in the URL. | |
* | |
* While History API does have `popstate` event, the only way to listen to | |
* changes via pushState and replaceState is using monkey-patching, which is | |
* what this function does. (See https://stackoverflow.com/a/4585031) | |
* It will fire a "pushState" or "replaceState" event, depending on which method was called. | |
* Setup events firing on history `pushState` and `replaceState` | |
* | |
* This is needed when addons are used in SPA. A lot of addons rely | |
* on the current URL. However in the SPA, the pages are not reloaded, so | |
* the addons never get notified of the changes in the URL. | |
* | |
* While History API does have `popstate` event, the only way to listen to | |
* changes via `pushState` and `replaceState` is using monkey-patching, which is | |
* what this function does. (See https://stackoverflow.com/a/4585031) | |
* It will fire a `READTHEDOCS_URL_CHANGED` event, on `pushState` and `replaceState`. |
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.
Do we need to make the distinction between these two events for our purpose? I think it will be better to expose just READTHEDOCS_URL_CHANGED
and always trigger this event either if it's pushState
or replaceState
. What do you think?
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.
Yeah, we can do it like that 👍
const originalMethod = history[methodName]; | ||
history[methodName] = function () { | ||
const result = originalMethod.apply(this, arguments); | ||
const event = new Event(methodName); |
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.
Here is where I think we should create new Event(READTHEDOCS_URL_CHANGED)
or similar.
How hard would it be to create a simple example for this using the I tested using |
I think I can make it work locally, to develop, and then I'll ask you to do a proper test on your setup |
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.
Changes here seem fine, no additions to the notes here 👍
Closes #157