1221821 - Support for Safari, Opera, Opera Mobile, iOS 9, Edge

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::L10n, defect)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

I'm working on a patch that makes us work in all ever-green browsers.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: pr

Firefox works with web version, Chrome works with compat version.

I'm adding changes to make compat version work in current versions Opera, Opera Mobile, Safari, Safari Mobile, Android browser, Edge.

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8683392 [details] [review]
pr

I verified this patch with Safari 9, iOS 9, Android 6.0, Chrome Mobile, Chrome, Edge 13, Opera Beta.

Comment 3

[Staś Małolepszy :stas]

Comment on attachment 8683392 [details] [review]
pr

I think we shouldn't be putting shims specific to src/lib in src/bindings/html/shims.js.  For instance, we'd want to the Intl shim to be available for context perf testing in Safari 9.  Can you put it into src/lib/shims?

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8683392 [details] [review]
pr

Updated to your comments

Comment 5

[Staś Małolepszy :stas]

Comment on attachment 8683392 [details] [review]
pr

Thanks!

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

https://github.com/l20n/l20n.js/commit/37681d6a21d626ae662bfde0e4f6643a87bedd83

Comment 7

[Staś Małolepszy :stas]

I need to revert this because it broke our non-transpiled code:

ReferenceError: can't access lexical declaration `Intl' before initialization

Comment 8

[Staś Małolepszy :stas]

Backed out: https://github.com/l20n/l20n.js/commit/487cfe68e086969a607d9faa9246bde220715bcd

Comment 9

[Staś Małolepszy :stas]

Zibi, this looks like the exact same thing that I backed out.  Forgot to push?

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8683392 [details] [review]
pr

>https://github.com/l20n/l20n.js/pull/102

Comment 11

[Staś Małolepszy :stas]

Comment on attachment 8683392 [details] [review]
pr

r- because (Intl || IntlShim) will throw a ReferenceError if Intl is not defined.  I suggest checking typeof Intl in src/lib/shims.

Comment 12

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8683392 [details] [review]
pr

Ok, since we're using it really in just one place, I opted for a simple if in the call. Let me know if that works for you.

Comment 13

[Staś Małolepszy :stas]

Comment on attachment 8683392 [details] [review]
pr

First of all, why is this attachment still pointing to pr 100 which was landed and then backed out?  Can you create a new attachment pointing to pr 102 and obsolete the old one, please?

Re. inlining the typeof check, I think there's value in separating the shimming logic into its own module, especially now that we don't have any perf tax on additional modules (yay rollup!).  Burying this logic deep in the src/lib code sounds like something that will come bite us back in the future.  My preference is then to put it in src/lib/shims, as you did before.

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: new PR

idk. Switching to new pr.

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

> My preference is then to put it in src/lib/shims, as you did before.

The reason I wanted to avoid that is that it requires us to introduce new variables carried around which increase complexity of the code and the amount of knowledge the user has to acquire to understand the code he's looking at.

In the inline case, assuming the user knows about Intl, he's good. In shim case, he also has to understand what the "IntlObject" is, find it imported, go to shims and see that it's the same as his good'ol Intl or a shim.

My position here is that shim makes sense when it can... shim. Which means it can make our code work as if the relevant part was implemented in scenarios when it's not.

But this `shim` does not work this way. It requires us to specifically import it (instead of adding support for the feature in the right place in DOM/scope) and it requires us to introduce a new arbitrary variable.

So, since we only use it in one place, I don't see how much it can "hit us later". If we'll start using it in more places, we can move it to this quasi-shim and it doesn't seem to me like something that will cost us a lot.

That's all I have for an argument. If you still prefer the shim, I'll do the shim.

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

Forgot to ni stas.

Comment 17

[Staś Małolepszy :stas]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #15) 
> The reason I wanted to avoid that is that it requires us to introduce new
> variables carried around which increase complexity of the code and the
> amount of knowledge the user has to acquire to understand the code he's
> looking at.

I guess we define code complexity differently.  Introducing new variables to make things clearer and easier to follow is something that I consider _reducing_ code complexity.  OTOH, entangling the shimming logic deep inside the calling code in src/lib/context _increases_ it, IMO.

> So, since we only use it in one place, I don't see how much it can "hit us
> later". If we'll start using it in more places, we can move it to this
> quasi-shim and it doesn't seem to me like something that will cost us a lot.

If this was about introducing 3 additional levels of abstraction to anticipate the future, I'd agree with you.  Here, I think it's a simple housecleaning which puts the shim in a separate module and we only have to remember to always use OurIntl.  But, as you say, we can change that later (famous last words? :).

Comment 18

[Staś Małolepszy :stas]

Comment on attachment 8687205 [details] [review]
new PR

lgtm, thanks.

Comment 19

[Zibi Braniecki [:zbraniecki][:gandalf]]

Landed: https://github.com/l20n/l20n.js/commit/24119b2aa342a188c8f6bbf50b40e189573c9107