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

Migrating RNTester to Packages Directory #29567

Closed
wants to merge 22 commits into from

Conversation

sansyrox
Copy link
Contributor

@sansyrox sansyrox commented Aug 4, 2020

Summary

Changelog

This PR aims to migrate the RNTester App to packages directory. But is currently, open to inspect the CI issues and resolve the merge conflicts.

Currently done

  • Working on iOS
  • Working on Android
  • Detox Tests working on iOS

Need to work on

  • Errors generated by the CI builds

[General] [Changed] - Migrated the RNTester App to the packages directory.

Test Plan

It runs on both ios and android for now and the detox iOS builds are working.

@sansyrox sansyrox requested review from cpojer and hramos as code owners August 4, 2020 22:27
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 4, 2020
@cpojer
Copy link
Contributor

cpojer commented Aug 5, 2020

You cannot call it "RNTester" if it is inside of the packages folder. It'll have to be either packages/tester or packages/rn-tester or maybe packages/test-app or similar.

@sansyrox
Copy link
Contributor Author

sansyrox commented Aug 5, 2020

@cpojer , okay got it. Should I proceed with packages/rn-tester then?
Also, do you want me to just change the folder name or the complete App Name, bundle name, etc. ?

@sansyrox
Copy link
Contributor Author

sansyrox commented Aug 6, 2020

@cpojer , I've made the suggested changes. I can squash the commits if the migration part looks good.

packages/rn-tester/Podfile.lock Outdated Show resolved Hide resolved
@@ -18,7 +18,7 @@ const RNTesterNavigationReducer = require('./utils/RNTesterNavigationReducer');
const React = require('react');
const URIActionMap = require('./utils/URIActionMap');

const nativeImageSource = require('../../Libraries/Image/nativeImageSource');
const nativeImageSource = require('../../../Libraries/Image/nativeImageSource');
Copy link
Member

Choose a reason for hiding this comment

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

This is a good example of the next step we'll take later to decouple RNTester from React Native. We should be able to set it up so that react-native resolves to '../../../, so that all of JavaScript references react-native, even if the native dependencies are still relative. Then we would work to treat the native deps the same way.

@@ -2,310 +2,312 @@
# yarn lockfile v1
Copy link
Member

Choose a reason for hiding this comment

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

Why did this change?

Copy link
Contributor Author

@sansyrox sansyrox Aug 8, 2020

Choose a reason for hiding this comment

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

Doesn't the yarn.lock file change automatically? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Not if the dependencies don't change.

@rickhanlonii
Copy link
Member

Going to import this just to see what's failing internally

@rickhanlonii
Copy link
Member

Importing isn't working for me, could you try @cpojer?

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @stealthanthrax in 63992c0.

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 Aug 20, 2020
facebook-github-bot pushed a commit that referenced this pull request Sep 30, 2020
Summary:
This PR fixes a small leftover after the RNTester migration to the monorepo package - an invalid link in the `PanResponder` comment.

Refs:
* #29567
* facebook/react-native-website#2177 (the similar correction was a part of merged PR in the RN docs)

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[Internal][Fixed] PanResponder: correct example link in comment

Pull Request resolved: #30051

Test Plan: N/A

Reviewed By: fkgozali

Differential Revision: D24025065

Pulled By: hramos

fbshipit-source-id: 190486e458fb8e83a35fa2d3c62f4483f3a4334d
philippeauriach pushed a commit to philippeauriach/react-native that referenced this pull request May 5, 2021
Summary:
## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->
This PR aims to migrate the RNTester App to `packages` directory. But is currently, open to inspect the CI issues and resolve the merge conflicts.

Currently done
 - Working on iOS
 - Working on Android
 - Detox Tests working on iOS

Need to work on
 - Errors generated by the CI builds

[General] [Changed] - Migrated the RNTester App to the packages directory.

Pull Request resolved: facebook#29567

Test Plan: It runs on both ios and android for now and the detox iOS builds are working.

Reviewed By: cpojer

Differential Revision: D23034761

Pulled By: rickhanlonii

fbshipit-source-id: e04bb06e1c7ef15d340206090d1575a871b9e6f5
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants