Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove nullish coalescing operator for mobile browser compatibility #28

Closed
wants to merge 1 commit into from

Conversation

tfitz237
Copy link

@tfitz237 tfitz237 commented Jan 5, 2021

The nullish coalescing operator is not available on some mobile browsers and it breaks the entire app for those browsers. This fix removes the operator and adds the truthy if statement which does the same thing.

The nullish coalescing operator is not available on some mobile browsers and it breaks the entire app for those browsers. This fix removes the operator and adds the truthy if statement which does the same thing.
@LukeChannings
Copy link
Owner

LukeChannings commented Jan 6, 2021

Optional chaining is available in all browsers this project supports (last 2 versions of Chrome, FF, and Safari) - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining#Browser_compatibility

Which browser in particular are you using?

@tfitz237
Copy link
Author

tfitz237 commented Jan 6, 2021

Oh sorry I thought I added that to the PR comment

Two different chromium browsers (Edge 45 and Bromite 68) didn't work.
On that page you linked it shows that FF mobile, Opera mobile are not supported.

@LukeChannings
Copy link
Owner

LukeChannings commented Jan 6, 2021

Edge 45 and Bromite 68 are both at least 2 years old now, and they also don't support other features that are relied on (Web Animations API won't work for sure) - I'm not very familiar with FF Mobile, but it doesn't appear to be up-to-date with Gecko in FireFox Desktop, and FF Mobile and Opera Mobile together account for less than half of 1% of market share.

I'm not trying to be obtuse, but browser support can be a slippery slope, and this is just a side project and I can't handle the additional maintenance cost of supporting older browsers.

Additionally, there are a bunch of feature requests that are being addressed in the dev branch and in order to accommodate the additional complexity I've rewritten the UI code in TypeScript (API types are shared between the F/E and B/E), so the toolchain has changed a bit (I'm compiling with esbuild).

Specific features like Nullish Coalescing / Optional Chaining can be compiled away, but platform APIs like Animations, Intl, etc. I don't want to polyfill.

I hope this doesn't discourage you from contributing again, I genuinely do appreciate the work, and welcome any contributions you're willing to make in the future.

@d1vanloon
Copy link

d1vanloon commented Jan 6, 2021

I respect that you've made your decision, but please understand that Edge 45 is the latest mobile (Android) version of Edge, so anyone using that browser on mobile (like me) will not be able to use this application in their default browser.

@LukeChannings
Copy link
Owner

LukeChannings commented Jan 6, 2021

Hey @d1vanloon,

I'll look into this more when I return home and can test on an Android device. My understanding is that Edge 45 Desktop is the last EdgeHTML / Chakra based browser, and future versions use Blink / V8.

On mobile, the latest version is 45, but is based on Blink / V8:

Instead of using the underlying Microsoft EdgeHTML rendering engine, the iOS Edge app will use Webkit like Apple does, and the Android Edge app will use the Chromium Blink engine.

Android is funny though, and there's a chance Microsoft chose to use the system's WebView instead of bundling its own Chromium version (a bad decision if true). Can you reproduce this bug on your Android phone with Edge?

@d1vanloon
Copy link

I haven't verified that this specific issue is affecting me, but I know that MovieMatch is not working in Edge mobile on my Android phone. I found this specific issue while investigating the problem I was having.

Edge mobile uses its own bundled version of Chromium, but it seems to be an old version. Edge 45 on my phone appears to be using Chromium 77.0.3865.116.

@LukeChannings
Copy link
Owner

LukeChannings commented Jan 6, 2021 via email

@tfitz237
Copy link
Author

tfitz237 commented Jan 6, 2021

I laugh here because all this PR does is remove a question mark. (But apparently after testing more there are more nullish operators you're using in other files so it's good this was closed)
I understand why you're rejecting it. Typescript compiling for the front end is smart. it means you can have all your fancy syntactic sugar (that does nothing but not make you type 3 more lines of code) and still support every browser just by telling it to compile to an earlier version.

I will just build the image myself with this change for my own purposes until you're dev branch is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants