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

Add custom hydration event for userscripts #2566

Merged
merged 1 commit into from
Jun 14, 2022

Conversation

mwiencek
Copy link
Member

Currently it's difficult for userscripts to determine when hydration is completed so that it's safe to modify the page. This adds a custom mb-hydration event that scripts can listen for.

Based on a suggestion by @ROpdebee in jesus2099/konami-command#596 (comment)

I tested this by creating a new Greasemonkey script with the following code and observing that several events were logged.

document.addEventListener('mb-hydration', (event) => {
  console.log('Got mb-hydration event:', event);
});

@mwiencek
Copy link
Member Author

@ROpdebee I can't request your review, but let me know if this looks okay. I know you also suggested additional hooks, so feel free to open tickets for other specific cases where an event is needed. (The tickets don't have to be super detailed, I'm just not sure what else is needed at the moment.)

Copy link
Contributor

@yvanzo yvanzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTMBDNT. As long as @ROpdebee is happy with it.

@mwiencek
Copy link
Member Author

This is now running on https://test.musicbrainz.org/

Copy link

@ROpdebee ROpdebee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new events do the trick, thanks! I tested both @jesus2099's INLINE STUFF and my affected userscript on test.MB.

I know you also suggested additional hooks, so feel free to open tickets for other specific cases where an event is needed.

I can't think of any at the moment, and I don't think I need any for my own scripts, but I'll keep an eye out for any places where they might be of added benefit over timeouts/intervals/mutation observers.

@mwiencek
Copy link
Member Author

I can't think of any at the moment, and I don't think I need any for my own scripts, but I'll keep an eye out for any places where they might be of added benefit over timeouts/intervals/mutation observers.

OK, sounds good. Thanks for testing!

Currently it's difficult for userscripts to determine when hydration is
completed so that it's safe to modify the page.  This adds a custom
mb-hydration event that scripts can listen for.

Based on a suggestion by @ROpdebee in
jesus2099/konami-command#596 (comment)
@mwiencek mwiencek force-pushed the custom-hydration-event branch from 14e67bf to 691fb0c Compare June 14, 2022 21:33
@mwiencek mwiencek changed the base branch from master to beta June 14, 2022 21:33
@mwiencek mwiencek merged commit 46827fb into metabrainz:beta Jun 14, 2022
@mwiencek mwiencek deleted the custom-hydration-event branch June 14, 2022 21:34
mwiencek added a commit that referenced this pull request Jun 14, 2022
* beta:
  Add custom hydration event for userscripts (#2566)
  Update POT files using the production database
  Update translations from Transifex
  Don't ISE on non-existing latest annotation
  MBS-12456: Load editor for latest_annotation on annotation page
  MBS-12455: Show annotation info when loading empty revision
  MBS-12453: Don't crash on null annotation comparison
  Use index, not ID, to enable/disable annotation comparison
@jesus2099
Copy link
Contributor

jesus2099 commented Jun 14, 2022

It looks interesting, @mwiencek!

Is it on beta?
Does it bubble all up to the <body>?
And also do you know if we have pages where hydration may happen not only once at the beginning but several times, for reasons?

@ROpdebee small warning.
Sometimes your script may run later than when the event is fired.
You should still, erh, I don't know, you know.
Maybe run the script when document-start, or something.

@mwiencek
Copy link
Member Author

mwiencek commented Jun 14, 2022

Hi @jesus2099,

Is it on beta?
Does it bubble all up to the <body>?
And also do you know if we have pages where hydration may happen not only once at the beginning but several times, for reasons?

It's on beta now, and does bubble up to the body and document.

Hydration can definitely happen many times on a single page. On release pages the event can trigger at least 5 times. So you'll have to check if the event target is the one you care about, e.g. div.tracklist-and-credits for the tracklist. Edit: But each event will be for a different section of the page. It should only trigger once per "root."

Sometimes your script may run later than when the event is fired.

I thought about adding a global variable that signaled whether hydration had already run for a set of elements, but even with @run-at document-end the userscript would always run before any MB scripts for me. Maybe document-idle would cause issues, or things are different outside of Firefox + Greasemonkey. If a global turns out to be needed, let me know.

@jesus2099
Copy link
Contributor

Thanks very much for everything!

@jesus2099
Copy link
Contributor

Ah, @mwiencek another question.
On recording merge page, for instance, where there is hydration to make the deferred display of AcoustID, for each recording row.
Will there be only a final mb-hydration event (on the table or tbody?) or one event for each recording row or for each AcoustID cell?

@ROpdebee
Copy link

Sometimes your script may run later than when the event is fired.

Ah, I didn't think of that. It seems like that can be detected by checking whether the root element has a property named _reactListening<randomness> whose value is true. It appears that that's added after hydration:

const tracklist = document.querySelector('.tracklist-and-credits');
console.log('Hydrated? ' + Object.keys(tracklist).some((prop) => prop.startsWith('_reactListening') && tracklist[prop]));

(so it turns out there was a way to determine if hydration had already happened…)

even with @run-at document-end the userscript would always run before any MB scripts for me

In my case, in Violentmonkey and Firefox with only one script enabled, it wasn't able to reproduce the issue at first. It always ran after MB scripts. It was only when I removed my jQuery @require that it started running before hydration (probably because it parsed much quicker).


This function should be able to handle both cases and should work in all browsers. Disclaimer: Untested.

function onReactHydrated(element, callback) {
  var alreadyHydrated = Object.keys(element).some(function (propertyName) {
    return propertyName.indexOf('_reactListening') === 0 && element[propertyName];
  });
  
  if (alreadyHydrated) {
    callback();
  } else {
    element.addEventListener('mb-hydration', callback);
  }
}

// Example
onReactHydrated(document.querySelector('.tracklist-and-credits'), function () {
  // Run the parts of the script that need hydration to have finished.
});

@mwiencek
Copy link
Member Author

Ah, @mwiencek another question.
On recording merge page, for instance, where there is hydration to make the deferred display of AcoustID, for each recording row.
Will there be only a final mb-hydration event (on the table or tbody?) or one event for each recording row or for each AcoustID cell?

Right now there is a different event for each AcoustIdCell. But this isn't a rule...it's technically possible to hydrate the whole table at once. It just depends on how we code it. Right now you can assume they're separate, but we can't promise it will never change. :)

@jesus2099
Copy link
Contributor

Thank you very much!
I will check if I can avoid setInterval daemons with this new event.

reosarevok added a commit that referenced this pull request Jun 20, 2022
* beta:
  Update translations from Transifex
  Add custom hydration event for userscripts (#2566)
  Update POT files using the production database
  Update translations from Transifex
  MBS-12311: Allow adding annotations to genres (#2492)
  Don't ISE on non-existing latest annotation
  MBS-12456: Load editor for latest_annotation on annotation page
  MBS-12455: Show annotation info when loading empty revision
  MBS-12453: Don't crash on null annotation comparison
  Use index, not ID, to enable/disable annotation comparison
  Add basic genre create/edit tests
  Update POT files using the production database
  Update translations from Transifex
  MBS-12395: Report for videos in mediums that shouldn't support video (#2562)
  MBS-12356: Correctly select + clean up Tidal store pages (#2515)
  Add comment to ensure test data is kept
  Save AC redirects before swapping AC uses
  Document and standardize Controller::Aliases tests
  Document and standardize Controller::EditAlias tests
  Document and standardize Controller::DeleteAlias tests
  Document and standardize Controller::AddAlias tests
  MBS-9188: Improve LinkedIn URL cleanup (#2553)
  MBS-12419: Block Genius.com links at release level (#2560)
  MBS-12417: Update Soundcloud cleanup to remove ? parameters (#2552)
  Avoid declaring my $tx twice in one test
  MBS-12351: Also trim space only disambiguations (#2510)
  MBS-12376: Don't show spammers on area pages (#2554)
  MBS-12447: Also show area-series rels on series page (#2563)
  MBS-12393: Change TOWER RECORDS to all-caps as per store Japanese usage (#2558)
  Remove useless Area::Create _insert_hash
  MBS-10165: Use edit system for genre editing
  MBS-10165: Use edit system for genre adding
  MBS-10165: Use edit system for genre deletion
  MBS-10165: Basic preparations for genre edits
  Support genres in formatEntityTypeName
  Simplify the allowed hosts shortener list
  MBS-12383: Block smart links: bfan.link
  MBS-12396: Block smart links: hyperfollow.com
  MBS-12401: Block smart links: hypeddit.com
  MBS-12350: Block smart links: bio.link
  MBS-12352: Block smart links: streamerlinks.com
  Add basic tests for the artist credit page
  Add IDs to sections of ArtistCreditIndex
  MBS-12354: Check if AC IDs are valid before passing to the DB
  MBS-12312: Convert edit.tt to React
  MBS-12312: Convert history.tt to React
  MBS-12312: Convert diff.tt to React
  MBS-12312: Convert revision.tt to React
  MBS-12312: Remove unused summary.tt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants