fix(link-assets): match asset extensions case-insensitively - #2847
Open
ahmdshrif wants to merge 1 commit into
Open
fix(link-assets): match asset extensions case-insensitively#2847ahmdshrif wants to merge 1 commit into
ahmdshrif wants to merge 1 commit into
Conversation
`link-assets` dispatches every asset to a per-extension bucket by comparing `path.extname()` against the lower-case type lists in `fileTypes.ts`. The comparison is case-sensitive, so an asset whose extension is cased differently on disk — `Lato-Regular.TTF`, `Photo.PNG` — never matches its bucket and falls through to the "custom" one instead. The result is a silent mislink: - on Android the font is copied to `app/src/main/assets/custom/` rather than `app/src/main/res/font/`, no `res/font/<family>.xml` is generated and no `ReactFontManager.addCustomFont()` call is added to `MainApplication`, and an image lands there instead of `res/drawable/`; - on iOS the file is added to the Xcode `Resources` group but `isFontAsset` is false, so it is never appended to `UIAppFonts` and the font ships inside the bundle without being loadable at runtime. Normalize the extension once, in `getAssetExtension()`, and use it both where `linkPlatform` builds its filters and in `migration2`, which flags previously linked fonts for relinking with the same comparison. Extensions that were already lower-case are unaffected: all 32 existing snapshots pass unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
link-assetsdecides how to link each asset by bucketing it on its file extension, inpackages/cli-link-assets/src/tools/linkPlatform/index.ts:fileExtcomes from the lower-case lists infileTypes.ts(otf,ttf,png,jpg,gif,mp3), so the comparison is case-sensitive. An asset whose extension is cased differently on disk —Lato-Regular.TTF,Photo.PNG,Clip.MP3— matches no bucket and falls through to thecustomone, which is meant for unknown file types.That is a silent mislink, not an error:
app/src/main/assets/custom/instead ofapp/src/main/res/font/; nores/font/<family>.xmlis generated and noReactFontManager.getInstance().addCustomFont(...)call is inserted intoMainApplication. An image lands in the same folder instead ofres/drawable/.Resourcesgroup, butcopyAssetsreceivesisFontAsset: false, so it is never appended toUIAppFontsinInfo.plist. The font is inside the app bundle and cannot be used at runtime.Uppercase font extensions are common (a lot of foundries and font sites ship
.TTF/.OTF), and this package's own README documents the supported types as "Fonts (OTF, TTF)" and "Images (JPG, PNG, GIF)", so.TTFis a reasonable thing for a user to have inassets/.The same case-sensitive comparison is in
tools/manifest/migrations/migration2.ts, which decides whether a previously linked asset is a font that needs relinking to XML resources, so an uppercase-extension font in an old manifest is skipped by that migration too.This normalizes the extension once, in a
getAssetExtension()helper next to the type lists, and uses it at both comparison sites. Assets whose extension is already lower-case are completely unaffected.Test Plan
Two tests were added to
packages/cli-link-assets/src/__tests__/linkAssets.test.ts, following the existing end-to-end pattern (a real temp project, real font fixtures,linkAssets()run for real):should link a font asset whose extension is upper-cased— linksassets/shared/fonts/Montserrat-Regular.TTFand asserts thatres/font/montserrat.xmlreferences it, thatres/font/montserrat_regular.ttfexists, that it is not inassets/custom/, thatMainApplication.ktgets theaddCustomFontcall, and thatInfo.plistlists it underUIAppFonts.should link an image asset whose extension is upper-cased— linksassets/shared/Upper Image.PNGand asserts it ends up inres/drawable/upper_image.pngand not inassets/custom/.Before the fix (tests applied, sources unchanged) both fail, showing the asset in the wrong place:
After the fix:
All 32 pre-existing snapshots pass unchanged, which is the regression guard for lower-case extensions.
Whole repo:
node_modules/.bin/jest packages→ 55 suites, 315 passed, 1 todonode ./scripts/buildTs.js→ cleannode_modules/.bin/eslint packages/cli-link-assets/src --ext .ts→ cleanChecklist
react-nativecheckout (instructions).