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

Make yarn and yarn jest react-native-codegen works on Windows with git #34854

Closed

Conversation

ZihanChen-MSFT
Copy link
Contributor

@ZihanChen-MSFT ZihanChen-MSFT commented Oct 3, 2022

Summary

A few fixings to make yarn jest react-native-doegen works on Windows:

  • Add a .gitignore file to tell git not to track generated/temporary files.
  • There is no rm on Windows, change it to rimraf.

I have been using it in the last 3 months and it works perfectly on Windows, otherwise I could not even build the code in my laptop.

Changelog

[General] [Changed] - Make yarn and yarn jest react-native-codegen works on Windows with git

Test Plan

yarn jest react-native-codegen passed

@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. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Oct 3, 2022
@analysis-bot
Copy link

analysis-bot commented Oct 3, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,739,292 -348
android hermes armeabi-v7a 7,141,935 -195
android hermes x86 8,050,381 -158
android hermes x86_64 8,022,197 +56
android jsc arm64-v8a 9,607,479 -384
android jsc armeabi-v7a 8,372,968 -217
android jsc x86 9,555,141 -185
android jsc x86_64 10,147,725 +40

Base commit: 87acdfb
Branch: main

@analysis-bot
Copy link

analysis-bot commented Oct 3, 2022

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

Base commit: 108c876
Branch: main

@kelset kelset added p: Microsoft Partner: Microsoft Partner labels Oct 4, 2022
Copy link
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

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

I am a bit concerned if this works also on unix-based system. But probably yes, given that it is a node module.

@facebook-github-bot
Copy link
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@robhogan
Copy link
Contributor

robhogan commented Oct 4, 2022

I am a bit concerned if this works also on unix-based system. But probably yes, given that it is a node module.

Yeah, rimraf is cross-platform

@cipolleschi
Copy link
Contributor

From @robhogan

What's this about? Does react-native-codegen write into its own directory?
If this doesn't need to be in the published package then it should probably be in the root .gitignore for consistency. If it does need to be in the published package, that's a bit worrying - it should probably be using something under os.tmpdir() if it needs to write to the filesystem.

Also, we have a linting error:

Files must end in a newline.
for the .gitignore

@ZihanChen-MSFT
Copy link
Contributor Author

ZihanChen-MSFT commented Oct 4, 2022

@cipolleschi

it should probably be using something under os.tmpdir() if it needs to write to the filesystem.

I remember they look like generated C++ files from the unit test, I don't know why these files are not checked in or something.

The rimraf is a different thing, it removes lib which are build result.

Also, we have a linting error:

Fixed.

@cipolleschi
Copy link
Contributor

I remember they look like generated C++ files from the unit test, I don't know why these files are not checked in or something.

This looks like something that only happens in Windows... Could you try again, by removing that tmp folder and see whether there are those tmp files? Perhaps it was some failed execution that left some dirt behind and we don't need to commit the .gitignore.

Also, if we need that, a better approach would be to update the .gitignore in the root folder

@ZihanChen-MSFT
Copy link
Contributor Author

@cipolleschi Hi, looks like this commit added the tmp folder to the root .gitignore so I think I don't need to submit this file now.

@facebook-github-bot
Copy link
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot pushed a commit to facebook/metro that referenced this pull request Oct 7, 2022
…h git (#34854)

Summary:
A few fixings to make `yarn jest react-native-doegen` works on Windows:

- ~~Add a `.gitignore` file to tell git not to track generated/temporary files.~~
- There is no `rm` on Windows, change it to `rimraf`.

I have been using it in the last 3 months and it works perfectly on Windows, otherwise I could not even build the code in my laptop.

## Changelog

[General] [Changed] - Make `yarn` and `yarn jest react-native-codegen` works on Windows with git

X-link: facebook/react-native#34854

Reviewed By: cortinico

Differential Revision: D40059524

Pulled By: cortinico

fbshipit-source-id: e3cde2506c7d18c2b580099257637b90f4cb328c
@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @ZihanChen-MSFT in c4f9556.

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 Oct 7, 2022
@ZihanChen-MSFT ZihanChen-MSFT deleted the ts-rncodegen-windows branch October 8, 2022 11:13
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
…h git (facebook#34854)

Summary:
A few fixings to make `yarn jest react-native-doegen` works on Windows:

- ~~Add a `.gitignore` file to tell git not to track generated/temporary files.~~
- There is no `rm` on Windows, change it to `rimraf`.

I have been using it in the last 3 months and it works perfectly on Windows, otherwise I could not even build the code in my laptop.

## Changelog

[General] [Changed] - Make `yarn` and `yarn jest react-native-codegen` works on Windows with git

Pull Request resolved: facebook#34854

Test Plan: `yarn jest react-native-codegen` passed

Reviewed By: cortinico

Differential Revision: D40059524

Pulled By: cortinico

fbshipit-source-id: e3cde2506c7d18c2b580099257637b90f4cb328c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. p: Microsoft Partner: Microsoft Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants