Skip to content

Fix Types for Platform.Version on Web #52360

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
riteshshukla04 wants to merge 3 commits into from
Closed

Fix Types for Platform.Version on Web #52360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3bc0b99
Fix Android Build
riteshshukla04 Jul 1, 2025
810e2a6
Fix Android Build
riteshshukla04 Jul 1, 2025
66708e3
Fix Android Build
riteshshukla04 Jul 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/react-native/Libraries/Utilities/Platform.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ type PlatformConstants = {
interface PlatformStatic {
isTV: boolean;
isTesting: boolean;
Version: number | string;
Version: number | string | undefined;
constants: PlatformConstants;

/**
Expand Down Expand Up @@ -94,6 +94,7 @@ interface PlatformWindowsOSStatic extends PlatformStatic {

interface PlatformWebStatic extends PlatformStatic {
OS: 'web';
Version: undefined;
}

export type Platform =
Expand Down
2 changes: 1 addition & 1 deletion packages/react-native/Libraries/Utilities/PlatformTypes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ type MacOSPlatform = {
type WebPlatform = {
OS: 'web',
// $FlowFixMe[unsafe-getters-setters]
get Version(): void,
Comment on lines 164 to -165

@NickGerleman NickGerleman Jul 1, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@riteshshukla04 riteshshukla04 Jul 1, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@necolas necolas Jul 1, 2025 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

@riteshshukla04 riteshshukla04 Jul 1, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do you mean something like "1000.0.0" always?

@necolas necolas Jul 1, 2025 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

@necolas necolas Jul 2, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

@cortinico cortinico Jul 11, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@necolas necolas Jul 11, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sounds good. I will update RNW as you proposed

@cortinico cortinico Jul 11, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the support @necolas 👍 Closing this PR then

@necolas necolas Jul 11, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

RNW follow-up issue: necolas/react-native-web#2785

get Version(): typeof undefined,
// $FlowFixMe[unsafe-getters-setters]
get constants(): {
reactNativeVersion: {
Expand Down
4 changes: 2 additions & 2 deletions packages/react-native/ReactNativeApi.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @generated SignedSource<<b56aa13d53b10140225651b9701de96d>>
* @generated SignedSource<<b484048eb6c9716bb6f5888a2b44ea78>>
*
* This file was generated by scripts/build-types/index.js.
*/
Expand Down Expand Up @@ -5850,7 +5850,7 @@ declare type WebPlatform = {
get isDisableAnimations(): boolean
get isTesting(): boolean
get isTV(): boolean
get Version(): void
get Version(): typeof undefined
}
declare type WindowsPlatform = {
OS: "windows"
Expand Down
Loading