Discourse does not render on gecko before version 74

Bug
1

commit 08883cbdd1c39a0c15b97af44ddaefe14acdc21a broke discourse rendering on older gecko releases. Gecko doesn’t support optional chaining operator before version 74.

See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining#browser_compatibility for more information.

2 Likes
2

Oh we are meant to be transpiling this, @pmusaraj / @eviltrout / @joffreyjaffeux ?

3 Likes
3

Might be wrong on this, but initial guess is that we don’t define browser list and it’s using some default (probably @babel/preset-env · Babel) and doesn’t consider this version as something we support and as a result is not transpiled.

It’s weird though firefox 74 is only one year old, but yes looking at browserlist currently:

$ npx browserslist
...
firefox 89
firefox 88
firefox 78
...

Now, the question is, @sam do we want to support this? Altough it’s only one year old, current fx version is 89, so this is 15 versions behind. Not sure about this your call. It’s important to consider that to support this it will add weight to everyone’s payload as transpilling basically works more or less like a polyfill (adding some functions tu support this and replace the syntax by functions calls).

2 Likes
4

(On stable this is UX: Add auto focus to hamburger and user menu dropdowns (#13165) · discourse/discourse@45dca79 · GitHub)

Out of curiosity, the old code style works well, why not stick to that for now? :thinking: FIX: We can't use `?.` yet (#13168) · discourse/discourse@96fd202 · GitHub

2 Likes
5

There are lot of reasons, but mostly it makes everything easier for dev, we don’t have to think too much about all the things we can use or not, and have a rather modern base.

4 Likes
6

I am open to dropping support provided our html fallback is in place.

What we can certainly not do is show a working static page to ie9 and a broken page to 1 year old Firefox.

I prefer to just transpile up to say 2-3 year old Firefox / Chrome, but am happy to leave that call to @eviltrout and you, we can go more aggressive I guess, but we can not display broken pages

Edit, see backlink, this can be a pretty serious issue on android that tends to have terrible cadence upgrading browsers

5 Likes
7

I couldn’t agree enough, and I do worry that browser compatibility has taken a few knocks lately.

Also on iOS there was this: Safari (iOS) shows blank pages on Discourse instances after beta 8 due to manually disable IntersectionObserver

1 Like
8

yep agreed, at minimum we should implement browserlist support so unsupported things dont come up as a surprise to us

3 Likes
9

Can this problem limit google indexing?

Just to figure out if I have to look elsewhere what is causing this indexing block.

Thanks

10

I can’t reproduce the OP on meta. I tried via Browserstack using Firefox 73 on macOS and Firefox 68 on Windows. @vige can you repro the problem on meta?

4 Likes
11

I think you are right it should actually work, I just looked at the code in the browser:

    toggleHamburger: function toggleHamburger() {
      this.state.hamburgerVisible = !this.state.hamburgerVisible;
      this.toggleBodyScrolling(this.state.hamburgerVisible); // auto focus on first link in dropdown

      (0, _runloop.schedule)("afterRender", function () {
        var _document$querySelect2;

        (_document$querySelect2 = document.querySelector(".hamburger-panel .menu-links a")) === null || _document$querySelect2 === void 0 ? void 0 : _document$querySelect2.focus();
      });
    },

Note that we should still try to know exactly what is getting transpiled or not, even though that might no be the issue here.

4 Likes
12

Ok correct if i’m wrong @pmusaraj but I think that as we don’t set preset-env and as a result don’t define targets, every plugins we include are used. As a result proposal-optional-chaining is getting applied for sure.

I think I’m ok with this, we maybe just need to check every year each plugin and reconsider if still needed?

2 Likes
13

Found it. Only stable is broken. proposal-optional-chaining is not getting applied there.

In tests-passed the ?. change includes the proposal-optional-chaining change: DEV: Enable optional chaining in all contexts (#13180) · discourse/discourse@855e854 · GitHub

and in stable only the code change with the ?. was made but the transpiler options were not adjusted: UX: Add auto focus to hamburger and user menu dropdowns (#13165) · discourse/discourse@45dca79 · GitHub

3 Likes
14

Yes good catch:

Capture d’écran 2021-06-09 à 18.21.51
Capture d’écran 2021-06-09 à 18.21.512448×566 88.1 KB

  • we added some code with optional chaining
  • we reverted it
  • we added it back with the correct babel plugin

However 5 days ago we backported only the original commit, without the plugin:

@pmusaraj I think backporting the plugin in stable should be safe too, don’t you think ?

8 Likes
15

Yes agreed, we should backport. I’ll work on it later today.

10 Likes
16

This is now fixed on stable (v2.7.4), the proposal-optional-chaining plugin for babel is now enabled there and it fixes the issue.

Thanks @RGJ @joffreyjaffeux (and @vige for the original report)!

12 Likes
17

Aha that makes so much sense! The code is in stable but the transpiler is not. Thanks for doing the detective work.

7 Likes
18

Thank you so much to all involved developers for fixing this so quickly!

For the record, according to my testing, this bug was also affecting, in some instances, the latest version of Microsoft Edge for Android (which, AFAIK, does not run Gecko).

6 Likes
19

Could you be so kind to add the actual v2.7.4 tag to that release?
I just spent half an hour trying to git checkout v2.7.4 which turned out not to exist :sweat_smile:

3 Likes
21

Sorry about that. It is now tagged.

7 Likes