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.

Sign up for GitHub

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

Fix Promise polyfill flow type #22048

Closed
wants to merge 1 commit into from
Closed

Fix Promise polyfill flow type #22048

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Libraries/Components/AccessibilityInfo/AccessibilityInfo.ios.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ const AccessibilityInfo = {
*
* See http://facebook.github.io/react-native/docs/accessibilityinfo.html#fetch
*/
fetch: function(): Promise {
fetch: function(): Promise<boolean> {
return new Promise((resolve, reject) => {
AccessibilityManager.getCurrentVoiceOverState(resolve, reject);
});
Expand Down
4 changes: 4 additions & 0 deletions flow/Promise.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,7 @@ declare class Promise<+R> {

static cast<T>(object?: T): Promise<T>;
}

declare module 'Promise' {
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 only needed because AccessibilityInfo is requiring the Promise module instead of using Promise from global. That seems weird. Does everything else use it from global?

Copy link
Contributor Author

@empyrical empyrical Nov 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It also fixes this Flow error that is showing up for me locally and some other people:

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ Libraries/Core/polyfillPromise.js:21:33

Importing from an untyped module makes it any and is not safe! Did you mean to add // @flow to the top of Promise?
(untyped-import)

     18│  * If you don't need these polyfills, don't use InitializeCore; just directly
     19│  * require the modules you need from InitializeCore for setup.
     20│  */
     21│ polyfillGlobal('Promise', () => require('Promise'));
     22│

I don't know why this error isn't showing up on CircleCI (I also did a clean re-install of all node_modules and this still appears)

Forgot to add that to the main PR message, sorry!

Copy link
Member

Choose a reason for hiding this comment

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

This fix seems detrimental to that particular issue. require('Promise') should be resolving to Libraries/Promise.js via Haste (which does have @flow at the top so there is something else weird happening). By specifying declare module 'Promise' { in flow/Promise.js it is bypassing the haste module which is probably not what we want.

Can you try running flow get-def on require('Promise') in the statement you have above and see where Flow is trying to resolve that module from and debug from there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's resolving to node_modules/promise/index.js.

The particular version of promise RN is using doesn't have a Flow type stub which is probably why this is showing up as any. The latest version (8.0.2) has it, however:

then/promise@c988089

A less ideal but simpler alternative however could be to just remove strict-local on polyfillPromise, since it's (supposed to be) the only place where promise is required directly.

(I am still not sure why it's working on CircleCI but not on my machine however 🤔 )

declare module.exports: typeof Promise;
}