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

Polish DevServerHelper #37266

Closed
wants to merge 2 commits into from
Closed

Conversation

cortinico
Copy link
Contributor

Summary:
This class is full of warnings and other issues which I'm doing a pass on it since I touched it:

  • Missing NonNull annotations
  • Try with resources missing
  • Unused inner Interfaces that can be removed

Technically a breaking change for users as we do have some public interfaces that have been removed, though not sure why people would depend on those.

Changelog:
[Android] [Removed] - Polish DevServerHelper (remove unused Interfaces)

Reviewed By: motiz88

Differential Revision: D45600284

@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 May 5, 2023
@facebook-github-bot
Copy link
Contributor

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

@analysis-bot
Copy link

analysis-bot commented May 5, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,717,740 +251
android hermes armeabi-v7a 8,028,519 +250
android hermes x86 9,205,095 +246
android hermes x86_64 9,058,480 +243
android jsc arm64-v8a 9,282,136 +117
android jsc armeabi-v7a 8,470,375 +109
android jsc x86 9,340,829 +113
android jsc x86_64 9,597,867 +112

Base commit: 8aa2281
Branch: main

cortinico and others added 2 commits May 5, 2023 08:49
Summary:
DevServerHelper was having a constructor parameter as `DevInternalSettings` which is effectively internal. This should not be the case as that class is Internal as was bleeding out of the public API.

I've updated the primary constructor to take instead:

```
  public DevServerHelper(
          DeveloperSettings settings,
          String packageName,
          InspectorPackagerConnection.BundleStatusProvider bundleStatusProvider,
          PackagerConnectionSettings packagerConnectionSettings) {
```

This is breaking change for users that were depending on the Internal class.

Changelog:
[Android] [Removed] - DevServerHelper should not depend on internal ctor parameter

Differential Revision: D45600283

fbshipit-source-id: 6cc07fed83355c1da27520cfc6ef73f9d9b03d6f
Summary:
Pull Request resolved: facebook#37266

This class is full of warnings and other issues which I'm doing a pass on it since I touched it:
- Missing `NonNull` annotations
- Try with resources missing
- Unused inner Interfaces that can be removed

Technically a breaking change for users as we do have some public interfaces that have been removed, though not sure why people would depend on those.

Changelog:
[Android] [Removed] - Polish DevServerHelper (remove unused Interfaces)

Reviewed By: motiz88

Differential Revision: D45600284

fbshipit-source-id: d0aae5ddad179a812e5b39ebbe0a3ce09b911bc8
@cortinico cortinico force-pushed the export-D45600284 branch from ba706c3 to 4087f09 Compare May 5, 2023 15:52
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label May 5, 2023
@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 7dcaf00.

jeongshin pushed a commit to jeongshin/react-native that referenced this pull request May 7, 2023
Summary:
Pull Request resolved: facebook#37266

This class is full of warnings and other issues which I'm doing a pass on it since I touched it:
- Missing `NonNull` annotations
- Try with resources missing
- Unused inner Interfaces that can be removed

Technically a breaking change for users as we do have some public interfaces that have been removed, though not sure why people would depend on those.

Changelog:
[Android] [Removed] - Polish DevServerHelper (remove unused Interfaces)

Reviewed By: motiz88

Differential Revision: D45600284

fbshipit-source-id: 6274ae29ff3384d7409764fd6474da68d777958a
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. 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.

3 participants