-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Use modern build for ipad os 15 and macos 11 #21695
Conversation
WalkthroughWalkthroughThe recent changes to the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
@@ -10,20 +10,29 @@ import { htmlMinifierOptions, terserOptions } from "../bundle.cjs"; | |||
import env from "../env.cjs"; | |||
import paths from "../paths.cjs"; | |||
|
|||
// Regex to match iPadOS >= 15 and macOS >= 11 (app web view) because browserlist is not capable of this | |||
const ipadMacOSRegex = | |||
/\b(iPadOS\s*(1[5-9]|[2-9]\d)(?:\.\d+)?(?:\.\d+)?)|(macOS\s*(1[1-9]|[2-9]\d)(?:\.\d+)?(?:\.\d+)?)\b/; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot hard code the versions because they can change with time and/or usage stats. I'm looking into the best solution here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be we should also put Safari 16 in browserlist file to have the same target in browserlist. I agree it's not perfect but I didn't find another solution 😢
Let's me know if you find something better!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've put in many hours researching this now, and the only solid conclusion I've got is that Apple needs to do better. 😞
For iPad, I think home-assistant/iOS#2918 is the best solution (and looks like @bgoncal just merged it as I'm typing so 🥳). There's no reason we need to allow the app to spoof the user agent to pretend it's a Mac because it won't affect the frontend result at all. So the current regex will work now for iPad at least.
For the Mac app, that's a complete mess. I had a discussion with @bgoncal and continued to research, and the only version provided by Apple's API is the OS version (i.e. there's no way to query what WebKit or Safari version gets used by the web view). And it's not provided in the default user agent string either. So the only choice is to map it ourselves. The only question for me becomes: should the MacOS version map to the Safari version it's shipped with, or the latest version it supports? Based on my research, I think the former is correct, but I'm not 100% sure on that.
I started looking into how to automate this similar to the way browserslist-useragent-regexp
works, and I think I could probably throw that together without too much trouble. It's late here now so I'll do that tomorrow.
Regarding putting more Safari versions in the browserslist
config, I agree. Currently, modern is served to iOS 15.6+ and Safari 17+, which doesn't make much sense from a transpilation/polyfilling perspective. That's just reflecting that iPhones are way more popular than Macs. We can hard code a sync for now and maybe I'll do another PR to browserslist
for a nicer solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree! Thank you for the research! 🙂
That was more or less my conclusion too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened browserslist/browserslist#838. I'll work on the change for Mac in a bit.
Closing in favor of #21724 (and since it's already fixed for iPad via home-assistant/iOS#2918) |
Proposed change
Following #16506
Partially fixes home-assistant/iOS#2908
The user agent from the iPadOS app or macOS app doesn't contain Safari version so it's ignored by browserlist.
As the app adds the os version in the user agent, I wrote a additional regex to have support for modern build for iPadOS >= 15 and macOS >= 11 (because they use Safari 16.6 engine).
@steverep Let me know if it's ok for you
Example of user agent from the app :
Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Home Assistant/2024.7 (io.robbie.HomeAssistant; build:2024.730; macOS 14.5.0) Mobile/HomeAssistant, like Safari
Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Home Assistant/2024.7 (io.robbie.HomeAssistant; build:2024.730; iPadOS 17.5.1) Mobile/HomeAssistant, like Safari
Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed: