Fix Types for Platform.Version on Web - #52360
Conversation
facebook-github-bot
added
CLA Signed
|
@riteshshukla04 can you fix the CI ? |
|
@cortinico CIs should be fixed now. |
| // $FlowFixMe[unsafe-getters-setters] | ||
| get Version(): void, |
There was a problem hiding this comment.
Should this be removed entirely?
This may also technically be a breaking change, for any user of Platform.Version(), since they now need to check platform before calling it, or get type-checker errors :/. But it seems more correct.
There was a problem hiding this comment.
Yes this is a breaking change. Platform can be possibly undefined. That was autogenerated file. I am not sure if removing get Version(): void, will be beneficial as we are keeping the key—even if it returns undefined.
There was a problem hiding this comment.
Could we make the value for the web type a stable string so the type matches native? I don't think this will ever need to be used in web, so may as well avoid changing the API
There was a problem hiding this comment.
Do you mean something like "1000.0.0" always?
There was a problem hiding this comment.
Yeah. I don't know if we have a web version of this module in RN. If we need something in the actual JS package, it could be 1000.0.0 or whatever value is used for iOS on native. I also assume a web target would be using RNfWeb, so that might be the place to add an actual version string if something specific is needed? The Platform module there doesn't even include Version yet and I don't remember getting any requests to add it - https://github.com/necolas/react-native-web/blob/0.20.0/packages/react-native-web/src/exports/Platform/index.js
There was a problem hiding this comment.
Let's see what @NickGerleman and @cortinico think. If they agree, I don't think we even need to patch RNfWeb yet because this part of the Platform API doesn't have a use on web that I know of.
There was a problem hiding this comment.
IMHO I'd rather update RNW to provide a fictional string and avoid the undefined type here (i.e. 0.0.0 might be a good one).
because this part of the Platform API doesn't have a use on web that I know of
@necolas agree that this APIs is not used directly inside RNW, but if react-native dictates that those fields should be implemented by the underlying platforms, we should make sure types are respected.
There was a problem hiding this comment.
Sounds good. I will update RNW as you proposed
There was a problem hiding this comment.
Thanks for the support @necolas 👍 Closing this PR then
There was a problem hiding this comment.
RNW follow-up issue: necolas/react-native-web#2785
Summary: This PR fixes #52356. As per discussion here #52360 . We have changed the implementation in React native web in this PR necolas/react-native-web#2791. As discussed with cortinico , Now web returns a hardcoded string "0.0.0" for `platform.version`. We can safely change this to string now. ## Changelog: <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests --> [GENERAL][CHANGED] Update types for Platform.version Pull Request resolved: #53216 Test Plan: N/A Reviewed By: christophpurrer Differential Revision: D80173301 Pulled By: necolas fbshipit-source-id: 750aae2427a6c36a068346a9722d9734d9906b58
5 participants
Summary:
Fixes #52356
Changelog:
[GENERAL][BREAKING]- Types for Platform.Version on Web
Test Plan:
N/A