Skip to content
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

React-based views don't correctly update features #6554

Open
fregante opened this issue Apr 25, 2023 · 10 comments
Open

React-based views don't correctly update features #6554

fregante opened this issue Apr 25, 2023 · 10 comments
Labels
ajax Temporary label to collect Ajax issues bug help wanted under discussion

Comments

@fregante
Copy link
Member

fregante commented Apr 25, 2023

I'm opening this issue to track it properly. In short:

  1. Open https://github.com/refined-github/refined-github/blob/main/source/features/action-used-by-link.tsx
  2. Visit a folder or change file via the sidebar or go back and forth

Known issues

  • turbo:render isn't called
  • popstate events don't trigger any turbo:render nor soft-nav:render
  • the view isn't generated from scratch, so previous elements stick around

Related issues

Current status

For the whole existence of Refined GitHub, we've relied on GitHub to delete our elements when the page/URL changes, with one exception (I don't remember which). New React-based views break this assumption. I think we need to:

  • listen to soft-nav:render
  • listen to popstate and avoid duplicates calls in case soft-nav:render is also fired
  • figure out a way to make widgets/buttons dynamic so that they change or disappear when the URL changes
    • this could help, but that alone doesn't fit into our concept of "feature unloading when pages change": A brief investigation of using SolidJS #6493
    • maybe we could add a onAbort(() => element.remove())
      • edit: I tried it, but if an rgh-seen element is still on the page, it won't be seen again.
@fregante fregante added help wanted under discussion ajax Temporary label to collect Ajax issues labels Apr 25, 2023
@fregante fregante added the bug label Apr 25, 2023
@fregante fregante mentioned this issue Apr 25, 2023
@134130

This comment was marked as outdated.

@fregante

This comment was marked as outdated.

@134130

This comment was marked as outdated.

@fregante

This comment was marked as outdated.

@fregante

This comment was marked as outdated.

@fregante
Copy link
Member Author

fregante commented Sep 7, 2023

In Chrome we can already use the navigation API

https://developer.mozilla.org/en-US/docs/Web/API/Navigation/navigate_event

@fregante
Copy link
Member Author

fregante commented Sep 11, 2023

Nope. It fires before the actual navigation, so we'd still have to wait for the content to be loaded somehow.

However I think I have a solution: .turbo-progress-bar

https://github.com/hotwired/turbo/blob/c44664d6acea224502d440754e2376635a6e9652/src/core/drive/progress_bar.js

  • This element is added to the page on click and removed on load
  • the element appears between head and body
  • it works on folders and files
  • the same element is reused

Detecting this removal might be a bit complicated, or maybe we just need a good ol MutationObserver on html, which should be lightweight

after

@fregante
Copy link
Member Author

I solved the URL detection issue in the PR above, but that's only part of it.

The main issue is that the DOM is no longer actually re-generated:

  1. our selector observer tags a "seen" element
  2. the contents page, but the element is never removed
  3. no add callback is ever fired

In practice this means our features are not updated. I think we just need dynamic elements:

  1. the feature is initialized with the current domloaded/turbo:load event
  2. use the selector observer to attach a React-ish element
  3. use the MutationObserver-based listener to detect changes to the view
  4. update the React-ish element

This brings us back to discussions like #6493, where we need to find a usable library and then work the new MutationObserver-based listener into it.

@fregante fregante removed their assignment Sep 11, 2023
@134130
Copy link
Contributor

134130 commented Sep 11, 2023

The main issue is that the DOM is no longer actually re-generated:

  1. our selector observer tags a "seen" element
  2. the contents page, but the element is never removed
  3. no add callback is ever fired

selector observer generates "seen" mark with caller id.
How about adding additional identifier which is based on location.href to "seen" mark?

If we can detect path changing correctly and ensure that feature is based on location.href, It will be easier and seamless way to support.

@fregante
Copy link
Member Author

fregante commented Sep 11, 2023

That's not going to be enough because we also need to remove the old element from the page. At that point it's easier to write:

  • on page unload, remove seen and element

Then the observer will take care of the rest, no need to add a dependency to the URL.

Maybe that's just what we need though:

  • have selector-observer remove the seen class when its signal is aborted
  • also add onAbort(signal, () => element.remove()) to these features

This wouldn't require writing dynamic elements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ajax Temporary label to collect Ajax issues bug help wanted under discussion
Development

No branches or pull requests

2 participants