docs: fix inaccurate readme claims and document bundle size - #91
Merged
Conversation
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.
Verified every claim against the published
phosphor-react-native@3.0.6, thereact-native-svgsource, andtsc --strict --skipLibCheck false.Tree-shaking claim (closes the README part of #61)
The README promised "Phosphor supports tree-shaking, so your bundle only includes code for the icons you use". Metro does not tree-shake by default, and the benchmark from #88 shows the barrel import costs +182% vs +10% for a direct import. Replaced with a Bundle size section carrying those numbers, plus the two real fixes: direct imports, or Expo's experimental tree-shaking.
Fixes
phosphor-react-native/lib/commonjs/icons/Staris not in theexportsmap — it fails withERR_PACKAGE_PATH_NOT_EXPORTEDon any resolver honouringexports, which Metro does by default since RN 0.79. Removed; documentedsrc/icons/*and its naming rule.interface SvgProps extends DefaultSvgPropsis self-recursive (TS2310) andDefaultIconPropswas never imported (TS2304). Rewritten as plain module augmentation, with a note on whyextendsis wrong. Same bug fixed inglobal.d.ts.onClick" —IconPropsis closed and RN has noonClick. Corrected the size-unit list:react-native-svgrunsparseInt, so"2rem"is 2, not 32. CorrectedduotoneColor, which falls back tocolor, not black. Added the missingstyleprop and per-prop defaults.Iconsuffix, and thatCircle/Path/Infinityhave no deprecated alias (1509 of 1512 do).phosphor-react->phosphor-icons/react,phosphor-icons->web, and so on).git submodule update --init --recursivestep, and links toCONTRIBUTING.mdandexample/.Verification
Every documented snippet typechecks against the published package under
--strictwithskipLibCheck: false— includingsize="50%", the duotone props,classNameafter augmentation, and thesrc/icons/Stardeep import. No formatting-only changes: the README was already not Prettier-clean onmain, andyarn lintcoverssrc/**only.Noted, not fixed here
src/lib/icon-base.tsx:48-49uses the rawweightprop instead ofweight ?? contextWeight, so an icon rendered without an explicit weight getstestID="phosphor-react-native-star-undefined"and a matchingclassName. Worth its own PR — I worded the README around it rather than documenting a wrong default.