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

RESOLVED FIXED

Status

Firefox OS
Gaia::L10n
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

40 bytes, text/x-github-pull-request
stas
: review+
Details | Review | Splinter Review
(Assignee)

Description

2 years ago
I'm working on a patch that makes us work in all ever-green browsers.
(Assignee)

Comment 1

2 years ago
Created attachment 8683392 [details] [review]
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.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
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.
Attachment #8683392 - Flags: review?(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?
Attachment #8683392 - Flags: review?(stas) → review-
(Assignee)

Comment 4

2 years ago
Comment on attachment 8683392 [details] [review]
pr

Updated to your comments
Attachment #8683392 - Flags: review- → review?(stas)
Comment on attachment 8683392 [details] [review]
pr

Thanks!
Attachment #8683392 - Flags: review?(stas) → review+
(Assignee)

Comment 6

2 years ago
https://github.com/l20n/l20n.js/commit/37681d6a21d626ae662bfde0e4f6643a87bedd83
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
I need to revert this because it broke our non-transpiled code:

ReferenceError: can't access lexical declaration `Intl' before initialization
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Backed out: https://github.com/l20n/l20n.js/commit/487cfe68e086969a607d9faa9246bde220715bcd
(Assignee)

Updated

2 years ago
Attachment #8683392 - Flags: review+ → review?(stas)
Zibi, this looks like the exact same thing that I backed out.  Forgot to push?
Flags: needinfo?(gandalf)
(Assignee)

Comment 10

2 years ago
Comment on attachment 8683392 [details] [review]
pr

>https://github.com/l20n/l20n.js/pull/102
Flags: needinfo?(gandalf)
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.
Attachment #8683392 - Flags: review?(stas) → review-
(Assignee)

Comment 12

2 years ago
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.
Attachment #8683392 - Flags: review- → review?(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.
Attachment #8683392 - Flags: review?(stas)
(Assignee)

Comment 14

2 years ago
Created attachment 8687205 [details] [review]
new PR

idk. Switching to new pr.
Attachment #8683392 - Attachment is obsolete: true
(Assignee)

Comment 15

2 years ago
> 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.
(Assignee)

Comment 16

2 years ago
Forgot to ni stas.
Flags: needinfo?(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? :).
Flags: needinfo?(stas)
(Assignee)

Updated

2 years ago
Attachment #8687205 - Flags: review?(stas)
Comment on attachment 8687205 [details] [review]
new PR

lgtm, thanks.
Attachment #8687205 - Flags: review?(stas) → review+
(Assignee)

Comment 19

2 years ago
Landed: https://github.com/l20n/l20n.js/commit/24119b2aa342a188c8f6bbf50b40e189573c9107
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.