Improves backwards compatibility for safari

[sejka]

fixes #1897

[sejka]

i have no idea why that build is failing :/

[sejka]

is it that tests are flaky or should i investigate more? @MichMich

[MichMich]

It is flaky indeed ... but somehow your PR is the only on consistently failing. Could you try rebasing your PR?

[sejka]

no luck, maybe there's something to be found after all 🤔 ? I'll look into it on Monday

[khassel]

your changes break the newsfeed module, it is not visible, no error messages ...

[sejka]

that's right! no idea why though. probably also doing some styling via style attributes 🤷‍♂ . Switched to using classList.add and .hidden class which should've been done anyway. Waiting for TravisCI to come back from maintenance.

[sejka]

@MichMich ready to merge

[MichMich]

Thanks. The reason why I haven’t merged yet is because I want to check out the code behind the header. If we aren’t going to display the header. Why add it to the dom? Adding a hidden class feels like a bandage on a issue that should be solved else where. There might be a good reason to do it this way, but I want to confirm before I merge this.

Thoughts?

[aghster]

I think this has been answered in the original issue #1897 (comment)

[aghster]

As it seems, the problem with the style property is that it is considered read-only. Correctly, one should not write directly to style, but set individual sub-properties of style. This means:

element.style = "display: none;";

should become

element.style.display = "none";

Older versions of Safari apparently are more strict in this regard.

[sejka]

I think @MichMich is more reffering to wheather headers should be in the DOM at all. I understand @tbouron intent with doing so. I'm just here to help people reuse older devices instead of throwing them away. Hopefully there is a way of having everyone happy.

[aghster]

In fully understand that @MichMich is checking whether unnecessary headers can simply not be created instead of being created and then hidden. With my above reference to @tbouron's comment I only intended to point @MichMich to why the current approach apparently had been implemented.

@sejka, I totally share your intention to improve compatibility with older browsers. I myself want to use MagicMirror in serveronly mode and use an old 3rd generation iPad running on iOS 9 as client. I tried to find out why some code does not work in Safari 9. This is what I have found out so far.

• As mentioned above, Safari 9 is strict with HTMLElement.style being read-only. Therefore one should replace element.style = "display: none;"; with element.style.display = "none";.
• Safari 9 does not understand ES6 arrow functions. They should therefore be replaced by ordinary functions.
• Safari 9 does not understand ES6 let statements. They should be replaced by var statements.
• Safari 9 does not understand ES6 concise method definitions as object properties. A definition such as obj = { foo(a, b) { … } }; should be replaced by obj = { foo: function(a, b) { … } };.
• Safari 9 does not understand ES6 default parameters. I suggest replacing function(param = default) { … } by function(param) { param = param || default; … }.
• Safari 9 does not understand class declarations. They should be replaced by ordinary class expressions.

Probably this list is incomplete. But in my fork (branch improve-compatibilty-with-old-browsers) of MagicMirror applying these replacements was enough to obtain a serveronly version running without apparent problems with Safari on my iOS 9 iPad as client.

I do not want to criticize or compete with your PR. I just wanted to share my results.

[buxxi]

Wouldn't it be much better to use a transpiler (like babeljs or something) for compability in older browsers than forcing the code to be stuck in the past (IE6 flashbacks...)?

[sejka]

great work @aghster!
I think in most cases we'd like to use newest features of JS and not make devs think twice before using a feature. Honestly I didn't think there's so many changes - that's why I've only changed that one thing.

I think we have two separate conversations going on - to have headers in the DOM despite them being empty and general backwards compatibility for browsers.

When it comes to second one i totally support @buxxi approach with babel.

[sejka]

looked into babel a little bit - i think we would need to ditch class.js for es6 classes though. Also found this issue: #694 which suggests that @MichMich isn't interested in complicating codebase that much. Maybe something changed though.

[buxxi]

Ditching custom class-creation for es6 classes doesn't sound like complicating it 😄

Guessing babel would be a part of the gruntfile if it would be used. Could also be part of a specific Dockerimage to avoid having the core of MagicMirror have to support older browsers (and require a build-step) since the main target is electron.

But there really should be something in the Contributing Guidelines about what browsers it needs to work in or what features of JS can/can't be used.

[MichMich]

Indeed. We will not use any pre-compilers. (see https://docs.magicmirror.builders/getting-started/manifesto.html) Sorry :)

[sejka]

I understand. What's your recommendation for users recycling their old devices for that use case? Should we create MagicMirrorLegacyDevices fork? Are we adding additional build step for legacy devices users? Maybe another docker image since most of older devices users are running host-client setup anyways?

[MichMich]

You can always run an older version of MagicMirror.

[PoojaAmin29]

Where will I get older version of magic mirror sorry for this silly question but I am new to this

…

On Wed, 1 Apr 2020, 13:13 Michael Teeuw, ***@***.***> wrote: You can always run an older version of MagicMirror. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1908 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJTYL3T65H2ZXVPRRC5KXH3RKLWBZANCNFSM4KMJ3XWQ> .

[MichMich]

Closing this for now because I don't see where we go with this. Feel free to reopen.