Skip to content
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.

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

Handle properly negative values in TS parser #34800

Closed
wants to merge 1 commit into from

Conversation

cipolleschi
Copy link
Contributor

@cipolleschi cipolleschi commented Sep 27, 2022

Summary:
The TypeScript parser was not handling negative default values properly. The reason why is because the AST for those values is structurally different and wraps them in a UnaryExpression with the - operator.

This Diff adds the support for those default values and it also add some tests in both Flow and TS.

Changelog

[General][Fixed] - Properly parse negative values

Differential Revision: D39847784

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner fb-exported labels Sep 27, 2022
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D39847784

@analysis-bot
Copy link

analysis-bot commented Sep 27, 2022

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 277a64f
Branch: main

@analysis-bot
Copy link

analysis-bot commented Sep 27, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,728,341 +0
android hermes armeabi-v7a 7,131,648 +0
android hermes x86 8,035,629 +0
android hermes x86_64 8,009,007 +0
android jsc arm64-v8a 9,596,969 +0
android jsc armeabi-v7a 8,363,291 +0
android jsc x86 9,541,199 +0
android jsc x86_64 10,133,601 +0

Base commit: 277a64f
Branch: main

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D39847784

cipolleschi pushed a commit to cipolleschi/react-native that referenced this pull request Sep 29, 2022
Summary:
Pull Request resolved: facebook#34800

The TypeScript parser was not handling negative default values properly. The reason why is because the AST for those values is structurally different and wraps them in a `UnaryExpression` with the `-` operator.

This Diff adds the support for those default values and it also add some tests in both Flow and TS.

## Changelog
[General][Fixed] - Properly parse negative values

Differential Revision: D39847784

fbshipit-source-id: 22e75b91ba3660ab2551478afeb9a60994263555
Summary:
Pull Request resolved: facebook#34800

The TypeScript parser was not handling negative default values properly. The reason why is because the AST for those values is structurally different and wraps them in a `UnaryExpression` with the `-` operator.

This Diff adds the support for those default values and it also add some tests in both Flow and TS.

## Changelog
[General][Fixed] - Properly parse negative values

Differential Revision: D39847784

fbshipit-source-id: 6885cb8eda0659e98aa208a43d027047082a2506
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D39847784

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @cipolleschi in f3c98c5.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Sep 30, 2022
kkafar added a commit to software-mansion/react-native-screens that referenced this pull request Jan 18, 2023
## Description

Issues

* [x] ~~`activityState` is set to `0.0` by default in codegened files
despite being explicitly set to `WithDefault<Float, -1.0>` in config (it
should be `-1.0`). Tested it multiple times it looks like you can't set
number < 0 for default when using TypeScript - tested with both `Int32`
and `Float` (with Flow it worked fine).~~
* [x] ~~`yarn lint` fails~~ 
* [x]
~#1631

There are no other differences between codegened files -- I checked it
file by file with diff tool.


Update: It was confirmed by React Native developer that this indeed is a
bug and will be fixed with 0.71.

Update:

* facebook/react-native#34800

was merged and is available is `0.71` rc branches of React Native. 

### ***Important note***

Merging this PR will be breaking change for Fabric support for
`react-native` versions lower than `0.71.0` (`activityState` &
`sheetCornerRadius` will be broken).

## Changes

* Migrated codegen spec files from Flow to TS.
* Also bumped `@types/react-native` to `0.69.6` [(see
explanation)](d2ed6ec)
* Had to do some minor type-fixing in our types
* Had to patch `react-navigation-stack` types. This dependency is
required for v4.

<!--

## Screenshots / GIFs

Here you can add screenshots / GIFs documenting your change.

You can add before / after section if you're changing some behavior.

### Before

### After

-->

## Test code and steps to reproduce

I created a backup of codegened code from before this PR. 
After applying these changes I ran: `./gradlew
:react-native-screens:generateCodegenArtifactsFromSchema` (or just build
application) and then I manually compared codegend files using diff
checker - the output code is the same.

## Checklist

- [x] Ensured that CI passes (there is a known issue with e2e on iOS, it
will be handled separately)
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
Pull Request resolved: facebook#34800

The TypeScript parser was not handling negative default values properly. The reason why is because the AST for those values is structurally different and wraps them in a `UnaryExpression` with the `-` operator.

This Diff adds the support for those default values and it also add some tests in both Flow and TS.

## Changelog
[General][Fixed] - Properly parse negative values

Reviewed By: cortinico

Differential Revision: D39847784

fbshipit-source-id: 95fc5768987477c540a54a7c4e4ff785d7a1e5d7
mccoyplayer pushed a commit to mccoyplayer/reactScreen that referenced this pull request Feb 9, 2024
## Description

Issues

* [x] ~~`activityState` is set to `0.0` by default in codegened files
despite being explicitly set to `WithDefault<Float, -1.0>` in config (it
should be `-1.0`). Tested it multiple times it looks like you can't set
number < 0 for default when using TypeScript - tested with both `Int32`
and `Float` (with Flow it worked fine).~~
* [x] ~~`yarn lint` fails~~ 
* [x]
~software-mansion/react-native-screens#1631

There are no other differences between codegened files -- I checked it
file by file with diff tool.


Update: It was confirmed by React Native developer that this indeed is a
bug and will be fixed with 0.71.

Update:

* facebook/react-native#34800

was merged and is available is `0.71` rc branches of React Native. 

### ***Important note***

Merging this PR will be breaking change for Fabric support for
`react-native` versions lower than `0.71.0` (`activityState` &
`sheetCornerRadius` will be broken).

## Changes

* Migrated codegen spec files from Flow to TS.
* Also bumped `@types/react-native` to `0.69.6` [(see
explanation)](software-mansion/react-native-screens@d2ed6ec)
* Had to do some minor type-fixing in our types
* Had to patch `react-navigation-stack` types. This dependency is
required for v4.

<!--

## Screenshots / GIFs

Here you can add screenshots / GIFs documenting your change.

You can add before / after section if you're changing some behavior.

### Before

### After

-->

## Test code and steps to reproduce

I created a backup of codegened code from before this PR. 
After applying these changes I ran: `./gradlew
:react-native-screens:generateCodegenArtifactsFromSchema` (or just build
application) and then I manually compared codegend files using diff
checker - the output code is the same.

## Checklist

- [x] Ensured that CI passes (there is a known issue with e2e on iOS, it
will be handled separately)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged This PR has been merged. p: Facebook Partner: Facebook Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants