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

[iOS][font] Stop scoping font family name in Expo Go #28344

Merged
merged 3 commits into from
Apr 21, 2024

Conversation

tsapeta
Copy link
Member

@tsapeta tsapeta commented Apr 20, 2024

Why

There is no more a reason to scope font family names in Expo Go on iOS. Fonts must be shared between different projects and scoping their names can only cause troubles. In this particular case, because scoped name contains a session ID, the same font loaded by home and NCL would have different font family aliases. As a result NCL is not able to unregister the previous font (from home) to register its own, i.e. it can't get inside this if:

// If the font was already registered, unregister it first. Otherwise CTFontManagerRegisterGraphicsFont
// would fail because of a duplicated font name when the app reloads or someone wants to override a font.
if let familyName = FontFamilyAliasManager.familyName(forAlias: fontFamilyAlias) {
try unregisterFont(named: familyName)
}

How

Modified fontFamilyNeedsScoping function so that it will never return true on iOS. I'm not sure what's the current situation on Android, but maybe we can remove scoping entirely (for sure too late for SDK 51).

Also excluded some tests on iOS that no longer make any sense. Looks like we could remove even more if we do a similar thing on Android (entirely remove describe('within Expo Go'))

To do once this PR is merged: publish dev home

Test Plan

  • JS tests are passing
  • Fonts and icons load properly in NCL on Expo Go

@expo-bot expo-bot added the bot: passed checks ExpoBot has nothing to complain about label Apr 20, 2024
@tsapeta tsapeta marked this pull request as ready for review April 21, 2024 10:13
@tsapeta tsapeta force-pushed the @tsapeta/ios/stop-scoping-font-family-name branch from 16c7053 to a128ae6 Compare April 21, 2024 10:19
@@ -1,3 +1,5 @@
import { Platform } from 'react-native';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be imported by expo-modules-core?

Copy link
Member Author

@tsapeta tsapeta Apr 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it doesn't matter much as long as we don't need to use properties that are not available in Platform from react-native. I'll change it though, for better consistency. Thanks for pointing this out!

@tsapeta tsapeta merged commit 70a34c8 into main Apr 21, 2024
3 checks passed
@tsapeta tsapeta deleted the @tsapeta/ios/stop-scoping-font-family-name branch April 21, 2024 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: fingerprint compatible bot: passed checks ExpoBot has nothing to complain about
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants