Skip to content

Fix storage/index.native.js to remove Platform.select#197

Merged
roryabraham merged 1 commit intomainfrom
Rory-FixPlatformFile
Oct 15, 2022
Merged

Fix storage/index.native.js to remove Platform.select#197
roryabraham merged 1 commit intomainfrom
Rory-FixPlatformFile

Conversation

@roryabraham
Copy link
Contributor

Details

Fixed a platform-specific file to comply with our guidelines. This was split off into a platform-specific file in this PR, but looking back I'm pretty sure that leaving Platform.select inside index.native.js was a mistake.

Related Issues

n/a

Automated Tests

n/a

Linked PRs

n/a

Testing plan

Create an E/App PR to upgrade the react-native-onyx version and test that the native storage still works (if it doesn't everything will break and we will surely notice). Also tag in an C+ for a thorough review on all platforms.

@roryabraham roryabraham self-assigned this Oct 11, 2022
@roryabraham roryabraham requested a review from a team as a code owner October 11, 2022 19:45
@github-actions
Copy link
Contributor

github-actions bot commented Oct 11, 2022

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@melvin-bot melvin-bot bot requested review from alex-mechler and removed request for a team October 11, 2022 19:45
@roryabraham
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@roryabraham
Copy link
Contributor Author

recheck

@roryabraham roryabraham requested a review from tgolen October 11, 2022 19:46
Copy link
Collaborator

@tgolen tgolen left a comment

Choose a reason for hiding this comment

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

Did you also test this in a web-only repo like the K2 extension to ensure it still works?

@roryabraham
Copy link
Contributor Author

Did you also test this in a web-only repo like the K2 extension to ensure it still works?

Nope, I'll do this and post screenshots before merging

@roryabraham
Copy link
Contributor Author

So I did some research and (in the Comp repo anyways), it turns out that the version of Onyx we are using is from June, 2021, and the latest version of Onyx (deployed to npm) does not work with Comp. So I wasn't able to test this upgrade in isolation, but was able to confirm that the problem was not introduced by this PR.

Furthermore, since this is an index.native.js file I think it's safe to merge and test with an E/App PR. We should create a follow-up issue to make sure that versions of Onyx we deploy to npm are compatible with web-only projects. I was seeing errors like this:

WARNING in ./src/pages/pair/comparison/ComparisonTable.js 267:15-23
export 'withOnyx' (imported as 'withOnyx') was not found in 'react-native-onyx' (possible exports: __esModule)
 @ ./src/pages/pair/ProfilePair.js 26:0-59 174:45-60
 @ ./src/pages/pair/PairPage.js 31:0-40 172:43-54
 @ ./src/pages/LoggedInPages.js 6:0-39 43:15-23
 @ ./src/Comp.js 20:0-50 169:70-83
 @ ./src/index.js 3:0-26 6:50-54

@roryabraham roryabraham merged commit 693e013 into main Oct 15, 2022
@roryabraham roryabraham deleted the Rory-FixPlatformFile branch October 15, 2022 15:18
@tgolen
Copy link
Collaborator

tgolen commented Oct 18, 2022 via email

@roryabraham
Copy link
Contributor Author

I created a follow-up issue here to investigate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants