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

JavaScript error: RangeError: invalid value unit for option style #26525

Closed
veita opened this issue Aug 15, 2023 · 18 comments · Fixed by #26575
Closed

JavaScript error: RangeError: invalid value unit for option style #26525

veita opened this issue Aug 15, 2023 · 18 comments · Fixed by #26575
Labels
issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail type/bug

Comments

@veita
Copy link

veita commented Aug 15, 2023

Description

Visit https://gitea.example.org/admin. The following error is shown on a red bar on the webpage:

JavaScript error: RangeError: invalid value unit for option style
(https://git.nezwerg.de/assets/js/webcomponents.js?v=1.20.2 @ 1:21550).

Gitea Version

1.20.2

Can you reproduce the bug on the Gitea demo site?

No

Log Gist

No response

Screenshots

No response

Git Version

git version 2.39.2

Operating System

Debian 12 Bookworm

How are you running Gitea?

Gitea binary downloaded from https://github.com/go-gitea/gitea/releases/download/ and run as a Systemd service.

Browser: Pale Moon 32.3.1.

Database

PostgreSQL

@veita veita added the type/bug label Aug 15, 2023
@silverwind
Copy link
Member

Can you show a screenshot of the surrounding JS code when you click the error line number in the browser console (F12)?

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Aug 15, 2023

The exception is thrown here:

https://github.com/github/relative-time-element/blob/d0da95b8c69bd22b76781d50d6143f7427844380/src/duration-format-ponyfill.ts#L113

What's your browser name/version and what's your locale (eg: run window.document.documentElement.lang in your browser console)?

(Hmm, I see Browser: Pale Moon 32.3.1, maybe it's a browser's bug)

And, are you using some browser plugins like translators? Does the error still exist in private window?

@wxiaoguang wxiaoguang added the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label Aug 15, 2023
@wxiaoguang
Copy link
Contributor

PaleMoon bug (incomplete Intl.NumberFormat support).

You could test it on this page https://github.github.io/relative-time-element/examples/ and fix the bug accordingly.

image

@veita
Copy link
Author

veita commented Aug 16, 2023

@wolfbeast
Copy link

It's not necessarily a "bug", it's just "yet another spec change" and "yet another tacked on API". We did lift our Intl support to the proper level recently, however we do not yet support the so-called Unified NumberFormat API which Gitea apparently now wants to use. i.e. not a bug, just another feature we'll have to implement.

Couldn't this have been implemented a bit more browser-agnostic in Gitea? I mean, not like "relative date formats" are a new thing, at all.

@wxiaoguang
Copy link
Contributor

Before 1.20: the "relative date" is rendered by backend, it doesn't support i18n well.

1.20: by using GitHub's "relative-time" element (#23988) , most date/time/duration texts are rendered by browser.

@wolfbeast
Copy link

wolfbeast commented Aug 18, 2023

Would you consider keeping the backend implementation as a fallback, at least? I mean, just hard throwing an error is always worse than "not supporting i18n well".

(as an aside, GitHub has indicated they want to always be on the bleeding edge of new client APIs, so using that as a reference for "what to implement" may not always be a good idea...)

@wxiaoguang
Copy link
Contributor

I am neutral for it, but I am not familiar with the "relative-time" change. Maybe the original author @yardenshoham could suggest.

@wolfbeast
Copy link

For the record, GitHub's relative duration implementation does seem to render correctly in Pale Moon.

@wxiaoguang
Copy link
Contributor

Maybe they use some internal packages or they use some polyfill packages.

@yardenshoham
Copy link
Member

Perhaps we could add some sort of polyfill for the missing NumberFormat API? I see a backend implementation as a step back in terms of evolution

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Aug 18, 2023

Just get an (somewhat strange) idea, would PaleMoon like to pack some polyfill packages by default? Then many browser features could be used out-of-box, instead of waiting for a C++ native implemention. 😁

@wolfbeast
Copy link

Just get an (somewhat strange) idea, would PaleMoon like to pack some polyfill packages by default?

That's rather complex and potentially dangerous as we can't just inject scripts into content willy-nilly, especially not if they are part of the browser's internal resources (it breaks the chrome/content boundary); on top, blanked polyfilling will conflict with other websites that may polyfill things themselves. In isolation it might be a good idea but not on the open web, unfortunately. We have an open issue to implement this API now, but no immediate timeline as to when we'll get to it.

@wxiaoguang
Copy link
Contributor

Made some tests, the minimal polyfill is like this (without i18n support):

	(function() {
		const old = Intl.NumberFormat;
		Intl.NumberFormat = function(locales, options) {
			if (options.style === 'unit') {
				return {
					format: function(value) {
						return ` ${value} ${options.unit}`;
					}
				}
			}
			return old(locales, options);
		};
	})();

image

@wxiaoguang
Copy link
Contributor

Add minimum polyfill to support "relative-time-element" in PaleMoon #26575

Does it look good or could there be better solutions?

@yardenshoham
Copy link
Member

@wxiaoguang
Copy link
Contributor

See the FAQ in #26575

image

@yardenshoham
Copy link
Member

I implemented https://formatjs.io/docs/polyfills/intl-numberformat/ with all locales, you are right the size will go from 60KB to 3MB, I wanted to only load the needed locale but that requires dynamic import which is asynchronous but we need it before we register the web component... Also we can't register the web component after the locale load because the HTML already has the relative-time

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail type/bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants