Fix "ref-issue" handling in markup

[wxiaoguang]

This is a follow up for #35662, and also fix #31181, help #30275, fix #31161

[wxiaoguang]

The "loading" state is still useful.

When I use GitHub, when my network lags, there is no UI response (no loading) when I hover the issue links, it looks unclear to me.

---

---

[Yakov5776]

The "loading" state is still useful.

When I use GitHub, when my network lags, there is no UI response (no loading) when I hover the issue links, it looks unclear to me.

GitHub doesn't highlight the issue if it doesn't exist though, so how does it look unclear?

As seen here:
Yakov5776/symlink-tests#1

[wxiaoguang]

The "loading" state is still useful.
When I use GitHub, when my network lags, there is no UI response (no loading) when I hover the issue links, it looks unclear to me.

GitHub doesn't highlight the issue if it doesn't exist though, so how does it look unclear?

I mean when my network lags, I didn't change the "issue not exist" case, it can be optimized separately.

[silverwind]

Can the fetch inside ContextPopup.vue be moved outside? I still prefer to use dumb "render only" components where possible, it was the original idea in #31181.

[silverwind]

on needs translation ideally.

[wxiaoguang]

Agree, ideally.

But I think it doesn't need to be in this PR's scope.

Handling i18n with datetime is also tricky, especially here it is done on frontend and mixes HTML elements.

Leave it to the future (and I don't see anyone complains)

[silverwind]

I think we dealt with on before related to relative-time-element, but it's probably all backend rendering there.

[wxiaoguang]

Nope, relative-time-element has dropped the "on" feature, it is non-translatable.

[wxiaoguang]

Can the fetch inside ContextPopup.vue be moved outside? I still prefer to use dumb "render only" components where possible, it was the original idea in #31181.

I don't think so. If you move it out, then the "loading" indicator state will be more complex. I believe the "loading" indicator is useful. And since we haven't resolved the "non-existing issue" problem, the "not found" error is also handled there.

I think the current approach is good enough

[silverwind]

If you move it out, then the "loading" indicator state will be more complex

Could just pass a loading: boolean prop to the component, but I guess it's not a big issue.

[wxiaoguang]

If you move it out, then the "loading" indicator state will be more complex

Could just pass a loading: boolean prop to the component, but I guess it's not a big issue.

Then how could you switch the "loading" prop? Vue doesn't work that way.

[silverwind]

Doesn't vue re-render when props change? At least that's how it works in React.

[wxiaoguang]

Doesn't vue re-render when props change? At least that's how it works in React.

If they are all in the reactive context, yes.

But here, the component is mounted from JS by a non-reactive props object. I don't think React works in this case either.

[wxiaoguang]

[silverwind]

Ah yes if you create a whole vue app or react root, then those props won't be reactive. The auto-update behaviour only works when a component renders another component (which is the typical case in a SPA).

[wxiaoguang]

Any other questions?

I think this PR is good enough, it resolves many legacy problems, the code is more maintainable, nothing becomes worse.

Although there could be some improvements, but none of them is easy to implement, so the improvements can be left to the future until someone really needs them and can use some right approaches to implement them.

[silverwind]

Instead of this, can you adjust globals.d.ts to add undefined to the property:

interface Element {
  _tippy: import('tippy.js').Instance | undefined;
}

And then just use el._tippy, typescript will then take care that the undefined case is handled.

[wxiaoguang]

It is very ugly.

I believe in we need to encapsulate the modules, but don't let caller use fragile code.

el._tippy is very fragile, it totally depends the undocumented 3rd party library behavior, should never be widely used.

[silverwind]

If you don't want direct el._tippy access, that's fine, but then these 2 cases should also use this function:

web_src/js/features/common-page.ts:62:              queryElems(elDropdown, '.menu > .item', (el) => el._tippy?.hide());
web_src/js/features/repo-issue-list.ts:189:        el._tippy.destroy();

[wxiaoguang]

If you don't want direct el._tippy access, that's fine, but then these 2 cases should also use this function:

web_src/js/features/common-page.ts:62:              queryElems(elDropdown, '.menu > .item', (el) => el._tippy?.hide());
web_src/js/features/repo-issue-list.ts:189:        el._tippy.destroy();

There are far more, these usages can be refactored later, not in this PR's scope.

[silverwind]

TBH, I find the typescript solution better, it does not necessitate such big refactors and is equally safe.

[wxiaoguang]

A variable starting with underscore already means it is for internal usage only, it is a widely accepted agreement across many different languages.

Using a variable with underscore prefix really doesn't seem good.

[silverwind]

It's a practical solution in case of tippy, I agree in a ideal world, properties should not be added to Element at all.

We should replace tippy (which is deprecated) with https://github.com/floating-ui/floating-ui.