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

[standards] Rewrite imports in RNTester to use standard paths #24317

Closed
wants to merge 2 commits into from

Conversation

ide
Copy link
Contributor

@ide ide commented Apr 4, 2019

Summary

See #24316 for the motivation. This commit rewrites the imports in the RNTester project.

Changelog

[General] [Changed] - Replaced Haste-style imports with standard path-style imports for RNTester

Test Plan

Run RNTester and verify that it loads without any issues. Tap through each screen to ensure the JS modules load (that is, were bundled) correctly.

@ide ide requested a review from cpojer April 4, 2019 23:44
@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: Expo Partner: Expo Partner labels Apr 4, 2019
@ide ide force-pushed the standard-paths-rntester branch 2 times, most recently from dd3aae8 to e70413d Compare April 4, 2019 23:47
Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • flow found some issues.

RNTester/js/RNTesterApp.ios.js Outdated Show resolved Hide resolved
RNTester/js/RNTesterExampleList.js Outdated Show resolved Hide resolved
RNTester/js/RTLExample.js Outdated Show resolved Hide resolved
RNTester/js/ScrollViewExample.js Outdated Show resolved Hide resolved
RNTester/js/TextExample.ios.js Outdated Show resolved Hide resolved
@ide ide force-pushed the standard-paths-rntester branch from e70413d to 7244c35 Compare April 5, 2019 00:12
@cpojer
Copy link
Contributor

cpojer commented Apr 7, 2019

Instead of the relative paths, what do you think about doing require('react-native/Libraries/…')?

Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • eslint found some issues. Run yarn lint --fix to automatically fix problems.

RNTester/js/AccessibilityExample.js Outdated Show resolved Hide resolved
@ide
Copy link
Contributor Author

ide commented Apr 10, 2019

  • Updated requires to use the public interface where possible (const {View} = require('react-native'))
  • Changed const ReactNative = require('react-native'); const {...} = ReactNative; to const {...} = require('react-native');, which sets us up to move to import/export more easily
  • I left requires of internal modules (utilities, for example) alone, since React Native may want to seal them off entirely outside of the Facebook repo and not encourage require('react-native/Libraries/Utilities/...'). React in particular publishes itself pre-bundled to intentionally separate its public interface from its internals and prevent require('react/...').

ide added 2 commits April 10, 2019 13:53
See facebook#24316 for the motivation. This commit rewrites the imports in the RNTester project.

Test Plan: Run RNTester and verify that it loads without any issues. Tap through each screen to ensure the JS modules load (that is, were bundled) correctly.
Instead of requiring specific files and bypassing the public interface, require react-native's exports where possible instead.
Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

Nice work. Thank you for doing this.

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.

@cpojer is landing 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 @ide in 26cce3d.

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 Apr 10, 2019
dsyang pushed a commit to dsyang/react-native that referenced this pull request Apr 12, 2019
Summary:
See facebook#24316 for the motivation. This commit rewrites the imports in the RNTester project.

[General] [Changed] - Replaced Haste-style imports with standard path-style imports for RNTester
Pull Request resolved: facebook#24317

Differential Revision: D14870504

Pulled By: cpojer

fbshipit-source-id: b14f22e7ce559efc332ced032617ca581196d90f
facebook-github-bot pushed a commit that referenced this pull request Apr 14, 2022
Summary:
This sync includes the following changes:
- **[8dcedba15](facebook/react@8dcedba15 )**: Add fallback shim for AbortController ([#24285](facebook/react#24285)) //<Ricky>//
- **[b86baa1cb](facebook/react@b86baa1cb )**: Add back lost cache test ([#24317](facebook/react#24317)) //<Ricky>//
- **[bafe912a5](facebook/react@bafe912a5 )**: update types for InputContinuousLane and DefaultLane ([#24316](facebook/react#24316)) //<Leo>//
- **[4ebaeae40](facebook/react@4ebaeae40 )**: moved mutation code to passive ([#24251](facebook/react#24251)) //<Luna Ruan>//
- **[caa60e8fc](facebook/react@caa60e8fc )**: update types for NonIdleLanes and IdleLane ([#24313](facebook/react#24313)) //<Leo>//
- **[1f7a901d7](facebook/react@1f7a901d7 )**: Fix false positive lint error with large number of branches  ([#24287](facebook/react#24287)) //<Stephen Cyron>//
- **[f56dfe950](facebook/react@f56dfe950 )**: Warn on setState() in useInsertionEffect() ([#24298](facebook/react#24298)) //<dan>//
- **[d68b09def](facebook/react@d68b09def )**: Fix warning about setState in useEffect ([#24295](facebook/react#24295)) //<dan>//
- **[057915477](facebook/react@057915477 )**: Update create-subscription README ([#24294](facebook/react#24294)) //<dan>//

Changelog:
[General][Changed] - React Native sync for revisions e8f4a66...8dcedba

jest_e2e[run_all_tests]

Reviewed By: kacieb

Differential Revision: D35581147

fbshipit-source-id: 33661d77eb000fdedab7e506a458fc739eab0056
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2023
Summary:
This sync includes the following changes:
- **[8dcedba15](facebook/react@8dcedba15 )**: Add fallback shim for AbortController ([facebook#24285](facebook/react#24285)) //<Ricky>//
- **[b86baa1cb](facebook/react@b86baa1cb )**: Add back lost cache test ([facebook#24317](facebook/react#24317)) //<Ricky>//
- **[bafe912a5](facebook/react@bafe912a5 )**: update types for InputContinuousLane and DefaultLane ([facebook#24316](facebook/react#24316)) //<Leo>//
- **[4ebaeae40](facebook/react@4ebaeae40 )**: moved mutation code to passive ([facebook#24251](facebook/react#24251)) //<Luna Ruan>//
- **[caa60e8fc](facebook/react@caa60e8fc )**: update types for NonIdleLanes and IdleLane ([facebook#24313](facebook/react#24313)) //<Leo>//
- **[1f7a901d7](facebook/react@1f7a901d7 )**: Fix false positive lint error with large number of branches  ([facebook#24287](facebook/react#24287)) //<Stephen Cyron>//
- **[f56dfe950](facebook/react@f56dfe950 )**: Warn on setState() in useInsertionEffect() ([facebook#24298](facebook/react#24298)) //<dan>//
- **[d68b09def](facebook/react@d68b09def )**: Fix warning about setState in useEffect ([facebook#24295](facebook/react#24295)) //<dan>//
- **[057915477](facebook/react@057915477 )**: Update create-subscription README ([facebook#24294](facebook/react#24294)) //<dan>//

Changelog:
[General][Changed] - React Native sync for revisions e8f4a66...8dcedba

jest_e2e[run_all_tests]

Reviewed By: kacieb

Differential Revision: D35581147

fbshipit-source-id: 33661d77eb000fdedab7e506a458fc739eab0056
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: Expo Partner: Expo Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants