search

mikemaccana / outdated-browser-rework

Detects outdated browsers and advises users to upgrade to a new version. Handles mobile devices!
MIT License
226 stars 63 forks source link

Edge v18 will be considered out of date? #67

Closed benmccallum closed 4 years ago

benmccallum commented 5 years ago

Just browsing the source and thinking that Edge versions you haven't got in the mapping dictionary will probably be considered out of date.

browserMajorVersion here will be undefined: https://github.com/mikemaccana/outdated-browser-rework/blob/2225b8ce5fa62377ef94f771024f6d0d7624f00d/index.js#L102

And then it's usage/comparison here against current version will always be false: https://github.com/mikemaccana/outdated-browser-rework/blob/2225b8ce5fa62377ef94f771024f6d0d7624f00d/index.js#L117

Could we add a special case for Edge to say if we don't know the mapping that it's an unknown browser? At least that way we can make sure the banner doesn't appear if we've got unknown browsers treated as OK.

mikemaccana commented 5 years ago

Could you just send me current mappings for Edge?

On Tue, 15 Jan 2019 at 8:44 am, Ben McCallum notifications@github.com wrote:

Just browsing the source and thinking that Edge versions you haven't got in the mapping dictionary will probably be considered out of date.

browserMajorVersion here will be undefined: https://github.com/mikemaccana/outdated-browser-rework/blob/2225b8ce5fa62377ef94f771024f6d0d7624f00d/index.js#L102

And then it's usage/comparison here against current version will always be false: https://github.com/mikemaccana/outdated-browser-rework/blob/2225b8ce5fa62377ef94f771024f6d0d7624f00d/index.js#L117

Could we add a special case for Edge to say if we don't know the mapping that it's an unknown browser? At least that way we can make sure the banner doesn't appear if we've got unknown browsers treated as OK.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mikemaccana/outdated-browser-rework/issues/67, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKiMlyEbrjE6LFDbgZHudJZRGuSvQjOks5vDZUJgaJpZM4aAfGV .

benmccallum commented 5 years ago

Looks like it's v44 to v18 according to https://en.wikipedia.org/wiki/Microsoft_Edge#EdgeHTML

benmccallum commented 5 years ago

Can submit a PR if you'd like, but thinking there's need for a more permanent fix? Although them swapping to Chromium might make this matrix end at some point.

mikemaccana commented 5 years ago

I'd rather have an up to date matrix rather than a work around for an out of date one.

pardon brevity, on phone.

On Tue, 15 Jan 2019 at 8:51 am, Ben McCallum notifications@github.com wrote:

Can submit a PR if you'd like, but thinking there's need for a more permanent fix? Although them swapping to Chromium might make this matrix end at some point.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/mikemaccana/outdated-browser-rework/issues/67#issuecomment-454312743, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKiMi2cej1iwahfnBv0S9D8Zmj9q1Ujks5vDZaOgaJpZM4aAfGV .

benmccallum commented 5 years ago

Yea, definitely need to keep it up to date, just a bit scary to have a new version be released and start showing users the banner haha. Like would be happening right now since Nov. Chucked up a PR, but will need dist compiled and new release. Cheers!

mikemaccana commented 5 years ago

Oh wait I think I see what you're saying here (deleted my last comment).

Are you saying that unlike other browsers, due to the EdgeHTML to Edge mapping, new versions of Edge that don't have entries in the map will be considered out of date?

mikemaccana commented 5 years ago

Yes re-reading your comment that's what you're saying. Sorry I misunderstood you.

Could we add a special case for Edge to say if we don't know the mapping that it's an unknown browser? At least that way we can make sure the banner doesn't appear if we've got unknown browsers treated as OK.

Yes please do, but please add a fat comment above the code explaining why!

benmccallum commented 5 years ago

Haha, thought I'd wait a few moments. I'll submit a PR when I get a sec.

Could probably also do something like find the latest version there is a mapping for, and at least use that to compare to a user's configured min version.

benmccallum commented 5 years ago

Started doing the changes yesterday evening but then wasn't sure how best to proceed.

Can I ask, why is there the mapping anyway? For more specific targeting? Do we need it, or could we support both styles of versions into two different config values?

If we still need it, how would you feel about using a mapped value if it's there, else "assuming" the version is "latest from mapping + 1"?

benmccallum commented 5 years ago

Hey @mikemaccana, if you have some time to provide some answers to above we can try tick this one off. Cheers.

mikemaccana commented 5 years ago

Can I ask, why is there the mapping anyway?

Because use browser versions, not their component versions. We don't care about Blink versions, Gecko versions, or Webkit versions, or EdgeHTML versions.

Do we need it, or could we support both styles of versions into two different config values?

No, we couldn't. It'd be a waste of code as nobody cares about EdgeHTML versions.

benmccallum commented 5 years ago

No worries. I think I'll go with my original idea here then of:

Could we add a special case for Edge to say if we don't know the mapping that it's an unknown browser? At least that way we can make sure the banner doesn't appear if we've got unknown browsers treated as OK.

Seems like the best option. Will submit a PR when I get a chance.

mikemaccana commented 5 years ago

We shouldn't detect unknown future EdgeHTMLs as unknown browsers. An newer (any bother browser) isn't treated as known, so we shouldn't do this with Edge

Check if the EdgeHTML reported by the browser is greater than the latest known Edge version in the mapping. Fill in a boolean IS_NEW_UNKNOWN_EDGEHTML accordingly. If that's true, force the browser version to the the latest in the mapping. This won't be entirely accurate, but still has the basic premise of treating newer browsers than thee versions we specify as good.

mikemaccana commented 5 years ago

Hah just realised this is still awaiting your PR. Wanna sent it?

benmccallum commented 5 years ago

Totally forgot about this, ahhh.. lemme see if I can whip that up real quick

benmccallum commented 4 years ago

Just testing this on new edge and it seems like it isn't an issue anymore as apparently undefined < minVersion is always false, so it was always considered up to date. I think with Edge now on Chromium, this can be closed, and we just need a new issue for handling new versions of Edge, which should be easy enough.