Repository navigation
fix(Settings): defer Platform lookup to fix circular dependency startup crash (#56967) - #57878
SnowingFox wants to merge 3 commits into
Conversation
Summary: Fixes react#56967. Settings.js evaluated Platform.OS at module load time, so when another module required Settings while Platform was still initializing (circular require), the Platform export was undefined and reading Platform.OS threw "Cannot read properties of undefined (reading 'OS')". Defer the platform lookup: resolve Platform lazily inside getSettings() on first method call, preserving the existing iOS vs fallback behavior for get/set/watchKeys/clearWatch. Changelog: [GENERAL] [FIXED] - Defer Platform lookup in Settings.js to fix circular dependency startup crash Test Plan: - yarn jest packages/react-native/Libraries/Settings/__tests__/Settings-test.js --maxWorkers=2 RED: circular-dependency test failed with "TypeError: Cannot read properties of undefined (reading 'OS')" at Settings.js:21 GREEN: all 4 tests pass (circular init, lazy Platform access, iOS delegation, fallback) - yarn jest packages/react-native/Libraries/Settings packages/react-native/Libraries/Utilities --maxWorkers=2 15 tests pass, no regressions Co-Authored-By: Claude <noreply@anthropic.com>
|
Hi @SnowingFox! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
Resolve the Settings.js conflict with the generic get and unknown-valued set signatures introduced by react#57736. Preserve the lazy Platform lookup, original regression tests, and existing commit history. Regenerate the public API snapshot for the SettingsStatic wrapper declaration. Validation: - Current main reproduces the circular initialization TypeError and eager Platform access regression; the resolved branch passes both. - Settings and Utilities Jest suites: 3 suites, 15 tests passed. - ESLint with --max-warnings=0 and Prettier on the PR files passed. - yarn build-types --validate passed after snapshot regeneration. - yarn test-generated-typescript passed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning JavaScript API change detected This PR commits an update to
This change was flagged as: |
Use the supported as-cast syntax for the cached implementation. Observe lazy Platform loading in the Jest mock factory instead of a side-effectful getter, preserving both regression assertions without a Flow suppression. Validation: - Reproduced both CI Flow errors before the change. - yarn flow-check: Found 0 errors. - Settings and Utilities Jest suites: 3 suites, 15 tests passed. - Revised circular and lazy-load tests fail against main and pass here. - ESLint --max-warnings=0 and Prettier checks passed. - yarn build-types --validate passed. - Generated and legacy TypeScript consumer checks passed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Summary:
Fixes #56967.
Settings.jsevaluatedPlatform.OSat module load time (a top-levelif (Platform.OS === 'ios')branch after importingPlatformfrom../Utilities/Platform). When another module requiredSettingswhilePlatform.ios.js/Platform.android.jswas still mid-initialization (circular require — e.g.NativePlatformConstantsIOSbeing loaded byPlatform.ios.js), thePlatformexport object was not yet populated, soPlatform.OSdereferencedundefinedand threw:This fix defers the platform lookup:
Platformis now required lazily insidegetSettings()on the first method call, instead of at module load. The public API is unchanged (get/set/watchKeys/clearWatchstill delegate to the iOS implementation on iOS and the fallback implementation elsewhere), matching the existingSettings.d.ts.Changelog:
[GENERAL] [FIXED] - Defer Platform lookup in Settings.js to fix circular dependency startup crash
Test Plan:
Added
packages/react-native/Libraries/Settings/__tests__/Settings-test.js(none existed) with 4 tests:NativeSettingsManager),RED — on the unmodified code, the circular-dependency test fails with exactly the reported bug:
GREEN — after the fix:
Regression check — Settings + Utilities suites (Platform lives in
Libraries/Utilities):Also verified:
eslinton the changed files exits clean (0 errors).