feat: re-implement ContextMenu & Menu

[Delusoire]

This is a fully backward-compatible rewrite of Context Menus, it doesn't rely on "injecting" HTML elements using Dom-manipulation or mutation observers.
Instead, it directly patches Spotify's react components for the context menu, at two different places, first to extract [uris, uids, contextUri, ... and expose them with a react context. And second, extend the children prop from the container.
This change makes the code significantly easier to understand, as it no longer relies on internal tippy features. It also fixes the long-standing issue of open Spotify sub-menus not closing after you hover over a Spicetify injected item.

[Delusoire]

With the last commit, the Profile menu is also handled in the exact same way as the Context Menu.

[SunsetTechuila]

I'm against:

1. having ContextMenu and ContextMenuV2
2. using webpack-loaded event

[Delusoire]

I'm against:

1. having ContextMenu and ContextMenuV2

ContextMenuV2 is temporary and should replace ContextMenu & Menu in the future.
I can hide it as an implementation detail for the moment but if you have a better idea do tell.

2. using webpack-loaded event

I have to disagree here, this is a perfect use case for using events, but I guess I can expose an onWebpackLoaded function that takes care of listening to the event & checking if it's already loaded.

[ohitstom]

may i ask if this rewrite will potentially solve this issue #2757?
or will this rewrite only include adding of individual items to the context menu and not creating an entire context menu and assigning to a dom element?

[SunsetTechuila]

ContextMenuV2 is temporary and should replace ContextMenu & Menu in the future.

In other words, you said yourself that this is something for Spicetify v3. I agree, which is why I'm against it

[SunsetTechuila]

I'm against using webpack-loaded event

nvm, just rename it to hooksDone or something

[Delusoire]

ContextMenuV2 is temporary and should replace ContextMenu & Menu in the future.

In other words, you said yourself that this is something for Spicetify v3. I agree, which is why I'm against it

This doesn't have to wait until V3, it is a drop-in replacement that works NOW. Why wait on a feature that doesn't break any existing API.

[Delusoire]

I'm against using webpack-loaded event

nvm, just rename it to hooksDone or something

Are you referring to this commit: 7ed19b9 ?
I think that the current nomenclature is more appropriate. The loading is non-linear because we rely on setTimeout madness, and as such when fireWebpackLoaded other Spicetify.*.* props are still not set.

Loading and order of execution for the different helper scripts and extensions should be revised in V3 to reduce the async mess, and not have extension developers check for whatever Spicetify.*.* they're trying to access in a recursive setTimeout.

[SunsetTechuila]

nvm, just rename it to hooksDone or something

Now I'm against onWebpackLoaded hook. Everything should be explicitly checked. Don't forget that Spicetify is a modding tool so nothing is reliable

Are you referring to this commit

No, I want the event to be renamed

Why wait on a feature that doesn't break any existing API.

Because no one will use it until we force them to. And I don't see a reason to do that.

Actually, all the breaking changes can be done without releasing v3 if we don't touch the current code and just add the SpicetifyV3 object. /s

[SunsetTechuila]

I'm against using webpack-loaded event

nvm, just rename it to hooksDone or something

No, as usual I got it right at once, but then mistakenly doubted myself. I'm against using the event too, since you don't check Spicetify.React explicitly

[Delusoire]

Why wait on a feature that doesn't break any existing API.

Because no one will use it until we force them to. And I don't see a reason to do that.

Not if they would be already using it; ContextMenu & Menu are like adapters, you can use them the old way and they translate to the newer implementation.

Why don't you want to wait on it? Because this implementation:

also fixes the long-standing issue of open Spotify sub-menus not closing after you hover over a Spicetify injected item.

[Delusoire]

I'm against using webpack-loaded event

nvm, just rename it to hooksDone or something

No, as usual I got it right at once, but then mistakenly doubted myself. I'm against using the event too, since you don't check if Spicetify.React explicitly

You're all over the place bro, I can't see where you stand.

So you'd rather have developers do:

than a simple:
Spicetify.Hooks.onWebpackLoaded(() => ...)
That is more reliable and executes as fast as the webpack Spicetify props are loaded.
You also don't have to interrupt the current execution stack to check if an object is finally not null every 10 milliseconds...

[SunsetTechuila]

than a simple: Spicetify.Hooks.onWebpackLoaded(() => ...)

as I said:

Everything should be explicitly checked. Don't forget that Spicetify is a modding tool so nothing is reliable

If you want to implement something like this, at least:

1. Rename the hook and event
2. Dispatch the event only if Spicetify.test() is executed without any errors

[rxri]

1. are all (specifically all) versions tested, 1.2.0 -> 1.2.29?
2. is the regex future-proof?
3. I don't like how you need to wrap stuff inside Spicetify.Hooks.onWebpackLoaded(() => {})
4. I agree with Grigory about:

> Why wait on a feature that doesn't break any existing API.

Because no one will use it until we force them to. And I don't see a reason to do that.

no other questions atm

[Delusoire]

than a simple: Spicetify.Hooks.onWebpackLoaded(() => ...)

as I said:

Everything should be explicitly checked. Don't forget that Spicetify is a modding tool so nothing is reliable

If you want to implement something like this, at least:

1. Rename the hook and event

There is no event.
The hook's name already reflects what it is / when it fires.

2. Dispatch the event only if Spicetify.test() is executed without any errors

And take a course on formal methods too? \s

Spicetify.test() SHOULD always run without errors, it is the maintainers responsibility to ensure that Spicetify's functionality is working, not the extension developer. The developer shouldn't care at all about whether this specific prop is loaded or not. The API SHOULD provide "signals" that indicate that all Network methods are loaded or all Webpack modules etc.

Spicetify.test() is only meant for testing environment / as a debugging tool. Not to be run for everyone on startup.

But if you insist on doing it your way, then you have to rethink how props are loaded. Probably something like: prop = assertNonNullAndReturn() and fire a groupA loaded event/callback at the end of the block, then try catch on the outer scope.

[Delusoire]

4. I agree with Grigory about:

> Why wait on a feature that doesn't break any existing API.

Because no one will use it until we force them to. And I don't see a reason to do that.

no other questions atm

Why wait on a feature that doesn't break any existing API.

Because no one will use it until we force them to. And I don't see a reason to do that.

Not if they would be already using it; ContextMenu & Menu are like adapters, you can use them the old way and they translate to the newer implementation.

Why don't you want to wait on it? Because this implementation:

also fixes the long-standing issue of open Spotify sub-menus not closing after you hover over a Spicetify injected item.

this ^
This is fixing a bug. And the bug SHOULD be fixed without a need for a new major release that's probably getting more delays than nasa's Artemis program.

[Delusoire]

3. I don't like how you need to wrap stuff inside Spicetify.Hooks.onWebpackLoaded(() => {})

And you'd rather have extension developers do this?

This is what they have to do in order to use 90% of the Spicetify wrapper objects.
And all this situation could have been prevented if the wrapper was split into two parts, and the other jsHelpers moved after the second part in index.html.
This is a symptom of how & when the jsWrappers are loaded, something that should definitely be reconsidered for V3.

[SunsetTechuila]

maintainers responsibility to ensure that Spicetify's functionality is working

But keeping spicetify installation updated is not

There is no event

Oh, right, you've removed the event and added a non-react hook. Don't understand why

The hook's name already reflects what it is / when it fires.

No, it is not. If it was, Spicetify.Hooks.fireWebpackLoaded() would be right after the window.webpackChunkopen check

[Delusoire]

An extension developer can still check if Spicetidy.. is defined inside the callback still!!

And use setTimeout if it is not..?

The step that's supposed to assign the prop failed...
That step is only done once...

ergo

The prop will never be assigned...

Extension developer can do as he pleases, but the prop will NEVER be available, they can be an idiot and fallback to a useless setTimeout, or define a fallback prop or panic or whatever.

[Delusoire]

Here is a low quality gif demonstrating the bug:

[SunsetTechuila]

The prop will never be assigned...

so all hooks are done after hotloadWebpackModules is done?

[Delusoire]

The prop will never be assigned...

so all hooks are done after hotloadWebpackModules is done?

Lmfao, wtf man. Can't you take a look at spicetifyWrapper.js?

onLoadedWebpackModules's callbacks are called when all the Spicetify props set inside hotloadWebpackModules are set.
Not when ALL SPICETIFY PROPS ARE SET. How many times do I need to say that for you to understand...

[Delusoire]

For example, Spicetify.Keyboard waits for Spicetify.Mousetrap which is exposed through webpackModules.
Reading Spicetify.Keyboard from inside a callback passed to onWebpackLoaded would always produce undefined.

[SunsetTechuila]

I was speaking about the hooks all the time, not about "all spicetify props"

But ok, this dialog has clearly reached an impasse

To summarize:

1. I don't like having ContextMenu and ContextMenuV2 at the same time. Already explained why: feat: re-implement ContextMenu & Menu #2776 (comment)
2. I want the hook to be renamed to onWebpackHooksDone. Or would be even better to create a separate PR for that hook. But it is ok if you don't want to

That's all rn

[Delusoire]

1. I don't like having ContextMenu and ContextMenuV2 at the same time. Already explained why: feat: re-implement ContextMenu #2776 (comment)

Solution that you proposed is simply to wait for V3 and release a breaking change.
My response was: This shouldn't have to wait because it's FIXING A BUG. And how stupid it is to hold off from fixing some bug because of a naming convention that can be settled in a matter of minutes. Also, V3 is nowhere near being production-ready, actually, V3 development has technically still not started (the V3 branch is like a tauri hello world) and no one seems to be motivated enough to pick it up.

2. I want the hook to be renamed to onWebpackHooksDone. Or would be even better to create a separate PR for that hook. But it is ok if you don't want to

Can't split the PR into two because the Menu changes make it depend on React, and the jsHelpers are run before Spicetify.React is loaded.
I'm not going to change the name of the dispatcher until everyone agrees on what to name it.

[ohitstom]

There is no event

Oh, right, you've removed the event and added a non-react hook. Don't understand why

First, hooks are in no way tied to react. They're a general concept that can be applied anywhere, and they predate react or any other js framework.

The event was simply an implementation detail.

would a non-react hook solve #2757
also this conversation is hilarious, sunset really doesnt know what hes talking about here

[Delusoire]

would a non-react hook solve #2757

Only way to use spotify's context menus is through the ReactComponent, so you have to use it. But if you don't wish to use react to generate the children dom tree then you have to use a useRef hook and create a div with react and ref passed to props. Then you can treat that ref as any other normal HTML element.

[ohitstom]

would a non-react hook solve #2757

Only way to use spotify's context menus is through the ReactComponent, so you have to use it. But if you don't wish to use react to generate the children dom tree then you have to use a useRef hook and create a div with react and ref passed to props. Then you can treat that ref as any other normal HTML element.

ah that makes sense, thank you! ❤️

[Delusoire]

Reopened as requested by @rxri.
The TLDR of the above discussion:

• The new implementation is more performant (as it doesn't rely on MutationObservers), and fixes 2 bugs, the first being:
  1. The incompatibility of Spotify's submenus with Spicetify-injected menu items.
  2. Spicetify's menu items were also injected into the add to playlist menu, triggered by clicking on the newly introduced plus sign.
• I sensed some confusion about the new ContextMenuV2:
  This pr exposes a new ContextMenuV2 object that serves as both an "infrastructure" and "preface" for the newer implementation, you can opt in to use it and get all the flexibility of modeling your UI with React directly (as if it was any other component written by Spotify as you get access to all the context providers etc...), or you can continue to use the older API, which internally translates every call to use ContextMenuV2 and get all the bug fixes.
• There was some backlash related to these new changes:
  1. "This could wait for V3": We know neither when V3 is going to get released nor how the new jsHelpers are going to be reworked.
  2. Spicetify.Hooks.onWebpackLoaded: This merely reflects a "ready" state, or rather a "webpack related props (a.k.a: Props defined from webpack modules) should have been ready by now". It takes a callback and calls it back when initialization of webpack related props is finished. It can easily be turned into a Promise as follows: await new Promise(resolve => Spicetify.Hooks.onWebpackLoaded(resolve)), which is a much more succulent alternative to:

      function main() {
          if (!Spicetify.React || !Spicetify.ReactDOM || !Spicetify.Mousetrap || /* all other deps */) {
              setTimeout(main, /* wasted arbitrary waiting time */)
              return
          }
          /* .... */
      }
      main()```

[afonsojramos]

Everything else looks alright, but I'll give it a re-review later

[rxri]

merging to prod without testing xoxo /s