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

Complete missing Flow declarations in URL #32566

Closed
wants to merge 1 commit into from

Conversation

mischnic
Copy link
Contributor

@mischnic mischnic commented Nov 9, 2021

Summary

When transpiling flow with a Babel config like

{
	plugins:  [['@babel/plugin-transform-flow-strip-types', {requireDirective: true}]]
}

all files containing Flow syntax need to have @flow at the top of the file.

node_modules/react-native/Libraries/Blob/URL.js: A @flow directive is required when using Flow annotations with the `requireDirective` option.
  55 |   _searchParams = [];
  56 |
> 57 |   constructor(params: any) {
     |                     ^^^^^
  58 |     if (typeof params === 'object') {
  59 |       Object.keys(params).forEach(key => this.append(key, params[key]));
  60 |     }

In URL, it was removed (mistakenly I think) by 6303850#diff-552f9731d5c9bc329105a5f4224319966395e59b5bb23202b756c5d152e0f007. Only the `strict-local´ part should have been removed, not the whole line.

Changelog

[General] [Fixed] - Complete missing Flow declarations in URL

Test Plan

See above

@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 Nov 9, 2021
@mischnic mischnic changed the title Add flow directive to Libraries/Blob/URL.js Add flow directive to URL and WebSocketInterceptor Nov 9, 2021
@analysis-bot
Copy link

analysis-bot commented Nov 9, 2021

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

Base commit: 36bbd8f
Branch: main

@lunaleaps
Copy link
Contributor

@mischnic It looks like this was also called out in the original PR: https://github.com/facebook/react-native/pull/22901/files#r246114640 but I guess if flow passes here then it's not an issue?

@facebook-github-bot
Copy link
Contributor

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

@analysis-bot
Copy link

analysis-bot commented Nov 9, 2021

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,477,542 -195
android hermes armeabi-v7a 7,774,787 -202
android hermes x86 8,946,689 -191
android hermes x86_64 8,893,604 -199
android jsc arm64-v8a 9,791,694 -88
android jsc armeabi-v7a 8,752,420 -81
android jsc x86 9,740,517 -80
android jsc x86_64 10,341,386 -82

Base commit: 36bbd8f
Branch: main

@mischnic mischnic force-pushed the patch-1 branch 2 times, most recently from fe79a27 to 4dbd4d5 Compare November 9, 2021 22:27
@mischnic
Copy link
Contributor Author

mischnic commented Nov 9, 2021

then it's not an issue?

Flow declarations without @flow are essentially dead code (and you can run into problems with other tools such as Babel or flow-remove-types).

Up until now I didn't actually test if Flow passed with my changes, so now I've:

  • removed the two "stray" type declarations in WebSocketInterceptor (that were ignored by Flow anyway and missing generic type parameters, now it contains no Flow declarations at all), because there are many missing types in that file
  • added all missing types in URL, so Flow passes now

@mischnic mischnic changed the title Add flow directive to URL and WebSocketInterceptor Fix flow directives for URL and WebSocketInterceptor Nov 9, 2021
@facebook-github-bot
Copy link
Contributor

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

@@ -17,8 +17,8 @@ const originalRCTWebSocketSend = NativeWebSocketModule.send;
const originalRCTWebSocketSendBinary = NativeWebSocketModule.sendBinary;
const originalRCTWebSocketClose = NativeWebSocketModule.close;

let eventEmitter: NativeEventEmitter;
let subscriptions: Array<EventSubscription>;
let eventEmitter;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was @flow removed from your last commit?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we leave this file untouched -- I realize the types could be dead but I'd prefer we do this in a separate PR so the intent is clearer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a separate PR

Done #32570

@mischnic mischnic changed the title Fix flow directives for URL and WebSocketInterceptor Complete missing Flow declarations in URL Nov 10, 2021
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Nov 10, 2021
@facebook-github-bot
Copy link
Contributor

@lunaleaps merged this pull request in 98abf1b.

@mischnic mischnic deleted the patch-1 branch November 10, 2021 21:25
facebook-github-bot pushed a commit that referenced this pull request Nov 11, 2021
Summary:
See #32566

This file doesn't have a `flow` comment, and the types that I removed here are incorrect anyway (missing a generic type parameter) because Flow didn't check them.

## 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
-->

[General] [Fixed] - Remove unused and incorrect type declarations in WebSocketInterceptor

Pull Request resolved: #32570

Test Plan: See #32566

Reviewed By: lunaleaps

Differential Revision: D32315077

Pulled By: GijsWeterings

fbshipit-source-id: d3b826a01a0bb8e38380de5b79cb6b5d13db2ef1
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. Merged This PR has been merged. Needs: Author Feedback 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.

5 participants