opnsense / core

OPNsense GUI, API and systems backend
https://opnsense.org/
BSD 2-Clause "Simplified" License
3.39k stars 759 forks source link

dashboard: switch to static loading of JS base class assets #7777

Closed Monviech closed 3 months ago

Monviech commented 3 months ago

Important notices

Our forum is located at https://forum.opnsense.org , please consider joining discussions there in stead of using GitHub for these matters.

Before you ask a new question, we ask you kindly to acknowledge the following:

Please reconsider the use of import maps for the new dashboard, as it breaks it completely for alternative browsers that do not support that feature (yet).

It seems like there are some OPNsense users who like to use privacy focused open source browsers.

An example is Palemoon (an open source Firefox alternative).

PR: https://forum.opnsense.org/index.php?topic=42231.0

Commit in question: https://github.com/opnsense/core/commit/c73f63c3fa7271f28192d66f8a6f1280e0ca0c2c

Feature: https://github.com/WICG/import-maps#readme

fichtner commented 3 months ago

Privacy on the firewall GUI? I know for a fact that some people have different or dedicated browsers to administrate systems. The real benefit of the feature is to avoid other caching workarounds. I don’t think it will be easy to argue against the bulk of the users not using these browsers here.

Just a few thoughts for discussion.

Cheers, Franco

fichtner commented 3 months ago

The compatibility list is quite extensive BTW. This one is even from Mozilla. Not sure where the Chrome hate is at. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script/type/importmap

wolfbeast commented 3 months ago

Browsers not necessarily on a commercial development path in terms of how quickly they can implement new features will be behind in support for otherwise unannounced changes to the HTML living standard (that changes daily). Even Mozilla indicated that this feature was complex even for their large team at time of implementation consideration:

From our side: at present, there isn't bandwidth to eagerly implement this proposal at the cost of other high priority work. Coupled with the experimental status, the shifting space of complex multi-resource applications, and the implementation complexity which may interfere with ongoing work, it doesn't make sense to implement immediately.

Goanna/Pale Moon/Basilisk/... is in a similar situation. While I understand this makes it easier for web admins to manage their JS module imports by not having to specify paths, implementing this in a browser engine is difficult and using it will inherently exclude browsers on a slower or more reactive development path. Yes, I'm aware the bulk of users will be on Blink-based browsers; but if you go by the argument of "the bulk is all we care about" then I wonder why anyone would even consider catering to Firefox, as well...

Privacy on the firewall GUI? I know for a fact that some people have different or dedicated browsers to administrate systems.

Many more do not, and would prefer a single browser instead of having to remember which browser to use for what.

Also note the docs pointed to list baseline 2023, i.e. it's a year old. Features with considerable complexity usually take much longer to implement in a releasable state.

Ultimately it's your choice, of course, but aside from a slightly easier to read module header without the paths in import statements, what would be the reason to use importmap instead of keeping broader compatibility?

doktornotor commented 3 months ago

what would be the reason to use importmap instead of keeping broader compatibility?

Well, as already written above:

The real benefit of the feature is to avoid other caching workarounds

I have nothing against Pale Moon or other Gecko forks, except for the fact the forking a web browser is not exactly a prototype of a good project for 5 people in a garage.

wolfbeast commented 3 months ago

the forking a web browser is not exactly a prototype of a good project for 5 people in a garage

Maybe not, but it depends entirely on the people involved. FTR: Waterfox was forked by a single person. Only big commercial players who never fall behind in anything allowed for Opnsense. Got it.

Monviech commented 3 months ago

I have nothing against Pale Moon or other Gecko forks, except for the fact the forking a web browser is not exactly a prototype of a good project for 5 people in a garage.

I think this is rather mean and it goes against the open source spirit. Remember how other projects started and where they were forked of. We should all look out for each other.

Edit:

What was the downside of this patch? It seemed to work too. https://github.com/opnsense/core/commit/314d975679afc9cf750a405c143495d551221b0a

fichtner commented 3 months ago

I'll say this once again for brevity: importmap is the best solution to the problematic caching issue in JS imports we had with the new dashboard. The compatibility rating is good. Not caching JS assets with 314d975679a was a dirty workaround. At the end of the day everyone is just asking someone else to do more work which we also have to carefully consider. ;)

fichtner commented 3 months ago

For anyone interested in the topic: We will ship https://github.com/opnsense/core/commit/04d9754006110bd7f6ef20a43149f21c269d4282 in the development version of 24.7.2 only. Please test and send feedback. Without feedback inclusion in 24.7.x is going to be unclear.

fichtner commented 3 months ago

I just want to remind everyone: no feedback at all here.

doktornotor commented 3 months ago

I still forgot to uninstall that 32bit Palemoon. Beyond opnsense-patch, anythiing else to be done to test it? Restart webgui?

fichtner commented 3 months ago

Restart shouldn't be needed.

doktornotor commented 3 months ago

Meh...

# opnsense-patch 04d9754
Fetched 04d9754 via https://github.com/opnsense/core
1 out of 1 hunks failed while patching opnsense/www/js/widgets/InterfaceStatistics.js
fichtner commented 3 months ago

Let me create a backport then, brb.

fichtner commented 3 months ago

https://github.com/opnsense/core/commit/e2bd3c33d -- note that plugin widgets fail because the code is out of sync, but all core widgets should show up fine.

doktornotor commented 3 months ago

That's a success - minus the "Add widget" dropdown which offers "Nothing selected". I believe this is the relevant F12 console noise:

TypeError: Argument 1 of Node.appendChild is not an object.
bootstrap-select.min.js:8:31958
TypeError: v is undefined
bootstrap-select.min.js:8:34703
fichtner commented 3 months ago

Do you have all core widgets on that dashboard? Might be an error when all widgets have been added.

doktornotor commented 3 months ago

Nope, not all. From Edge:

image

fichtner commented 3 months ago

I'll have the best man @swhite2 on the job.

swhite2 commented 3 months ago

@doktornotor I guess the symptom is not being able to select a dropdown entry?

Any mention of opnsense_widget_manager.js with line numbers in the resulting stack trace in the browser console?

doktornotor commented 3 months ago

The one line there appears to be due to the telemetry widget, not sure?

Yes, it was empty. Logged out, wiped cache, logged in again, and, it started working - once. Now, I deleted a widget and tried to re-add it. Guess what, the dropdown is empty again.

Besides that, I seem unable to drag and drop the widgets.

Thanks for troubleshooting this, but I do not have a use case, merely offered to quickly test things as it seemed trivial enough. That is apparently not the case and frankly more work than this the extremely rare use case requires, IMO.

Here's a screenshot of the dev console from random tinkering with the dashboard if it's of any use.

image

swhite2 commented 3 months ago

The one line there appears to be due to the telemetry widget, not sure?

Correct, the patch does not apply to the plugins part and since we import regardless of current usage it's probably best to manually edit the plugins .js files like https://github.com/opnsense/plugins/commit/1ea9867ac934c7db34893f5b84d8c34be79c39e5

My guess is the issue's solved after that, but would like to know either way.

Thanks for taking the time to test :)

doktornotor commented 3 months ago

Removed plugin widgets (telemetry, DynDNS). Wiped cache. The dropdown seems to be working. Adding, removing widgets seems to be working. Dragging / resizing is broken.

Then there's still:

image

Deleted the Traffic Widget. Wiped cache. Reloaded.

image

After that, dragging / resizing the widgets still does not work. I don't believe there are any plugin widgets left.

Conclusion: the browser sucks. Moving on. If someone has interest in this, those users needs to spend their own time debugging and testing. So far the only user who complained about Palemoon posted something like "I don't use dashboard any more, ktnxbye". 🤷‍♂️

meyergru commented 3 months ago

As I am all for omitting vendor-specific "standards" like importmap, I tested if there are regressions on this.

Tried "opnsense-patch e2bd3c33d" with Firefox using the default dashboard, which seems to work fine. On my production system, where plugin widgets are defined, dashboard stays empty (as was to be expected).