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

Clean .flowconfig so users won't need configuration to use fbjs #182

Merged
merged 5 commits into from
Sep 21, 2016

Conversation

gabelevi
Copy link
Contributor

@gabelevi gabelevi commented Sep 7, 2016

Currently, if you just do

$ mkdir foo
$ cd foo
$ npm init -yes
$ npm install fbjs
$ flow check

You'll get the following errors

node_modules/fbjs/lib/Deferred.js.flow:60
 60:     Promise.prototype.done.apply(this._promise, arguments);
                           ^^^^ property `done`. Property not found in
497: declare class Promise<+R> {
     ^ Promise. See lib: /private/tmp/flow/flowlib_20fe24e/core.js:497

node_modules/fbjs/lib/shallowEqual.js.flow:29
 29:     return x !== 0 || 1 / (x: $FlowIssue) === 1 / (y: $FlowIssue);
                                   ^^^^^^^^^^ identifier `$FlowIssue`. Could not resolve name

This is not great for users, and I've reaped a bunch of stack overflow karma from this problem.

In the future, we'll update the publish process to use flow gen-flow-files to generate the .js.flow files. This way, fbjs will still be published with types, but not with the actual original source code.

For now, the simplest solution is to just go through the .flowconfig and remove as much as possible. That way, you won't get errors when the users don't have the same configuration.

@ghost ghost added the CLA Signed label Sep 7, 2016
@@ -57,7 +57,7 @@ class Deferred<Tvalue, Treason> {
}

done(): void {
Promise.prototype.done.apply(this._promise, arguments);
Promise.prototype.then.apply(this._promise, arguments);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not 100% sure about this change. It seems equivalent, after looking at the definition on https://www.promisejs.org/api/ (which also includes the polyfill). done() seems like then(), except it doesn't return a Promise, so you can't chain.

If I'm wrong, I'll just change this to

(Promise.prototype.done: any).apply(this._promise, arguments);

but I thought I'd rather try this first.

Copy link
Member

Choose a reason for hiding this comment

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

done() also throws, which is a big difference.

@gabelevi
Copy link
Contributor Author

gabelevi commented Sep 7, 2016

I tested this by doing

$ mkdir foo
$ cd foo
$ npm init -yes
# Install my local edited version of fbjs
$ npm install ../fbjs-gabe/packages/fbjs
$ flow check

And got 0 Flow errors :)

@zpao
Copy link
Member

zpao commented Sep 7, 2016

These are synced from www and changes will get overwritten unless carefully managed (there are 2 other cases we do this so it's not out of the question but something to keep in mind). If possible, I'd like to make sure we make any changes required internally as well to avoid the issue, or figure out if there's anything else we can do here to avoid modifying these (at least in src form, free range for build-time).

For the Promise issue, I was thinking of adding type imports for the build step so that Deferred would import our Promise as opposed to the Flow one, presumably bypassing the problem as we'd explicitly be using a different Promise impl.

@gabelevi
Copy link
Contributor Author

I've updated this PR to include the throwing behavior of done() and I've sent an internal diff updating shallowEqual.js and Deferred.js. Once that lands and sync's to GitHub, I'll rebase this on top of that.

@zpao
Copy link
Member

zpao commented Sep 14, 2016

Can you just update the files here when that lands? We don't have autosyncing and I haven't gotten around to fixing some scripts now that static_upstream has changed.

Edit: oh you already did 👍. Thanks! Let's just make sure any other changes before it lands make it out too.

@gabelevi
Copy link
Contributor Author

The internal diff landed. I made the last few changes here and then cleaned up the commit history. Let me know if you prefer me to squash everything, though.

@zpao zpao merged commit 4113922 into facebook:master Sep 21, 2016
@zpao
Copy link
Member

zpao commented Sep 21, 2016

I guess we probably want this to make it's way out to more current versions of React (and relay, draft) right? We'd need to do a branch release for that which is totally doable, just want to know what you think we should do.

@itajaja
Copy link

itajaja commented Sep 26, 2016

Do you guys have a timeline for when the next release with this fix is going to be cut?

@gabelevi gabelevi deleted the flow branch September 26, 2016 22:41
zpao pushed a commit that referenced this pull request Sep 27, 2016
* Update to Flow v0.32.0

* Get rid of Flow suppress_type

* Get rid of flow lib by inlining Promise.prototype.done polyfill

* Get rid of unused include

* Get rid of unused ignore

(cherry picked from commit 4113922)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants