Type Transaction #7581
Type Transaction #7581
Conversation
ghost
added
| @@ -39,7 +39,7 @@ function warnNoop(publicInstance: ReactComponent<any, any, any>, callerName: str | |||
| * @param {Transaction} transaction | |||
| */ | |||
| class ReactServerUpdateQueue { | |||
| /* :: transaction: Transaction; */ | |||
| transaction: Transaction; | |||
|
|
|||
| constructor(transaction: Transaction) { | |||
There was a problem hiding this comment.
since Transaction was not flowified, this didn't do anything!
There was a problem hiding this comment.
The flow team wants to make this an error in the future. We shouldn't be able to use a non-flow file as type as it silently turns it into any. (Internal discussion: https://www.facebook.com/groups/flowtype/permalink/1132421916806422/ )
vjeux
force-pushed
the
type_Transaction
branch
2 times, most recently
from
b575dfb to
72db636
Compare
|
Ok, updated with a different proposal: it turns out that no one uses the second export (OBSERVED_ERROR ). So, instead of exporting an object with a "Mixin" (which is confusing as hell to me), I return the So, now for the call site it is super simple, if you want the implementation you do var Transaction = require('Transaction');if you want the type you do import type { Transaction } from 'Transaction';It's a little bit weird inside of the file as I need to name Transaction something else because the export would overshadow the name. I chose What do you think @sebmarkbage? |
This one is interesting because we have transaction objects being passed around everywhere in the codebase but there's actually no Transaction class. It's a "mixin" that comes to life by being Object.assigned to the prototype of a real "class" (before class was cool!). Therefore, we can't just say `var Transaction = require('Transaction'); (transaction: Transaction) => { }` because it would be the object that contains a mixin and not an instance of a transaction.
The trick I use is to export `TransactionType` and alias it to `Transaction` in the file as it doesn't actually require transaction. In case they do, we'll figure it out, but in the few files I looked at, it doesn't seem to be the case.
For the perform function, it actually typechecks pretty well!
vjeux
force-pushed
the
type_Transaction
branch
from
72db636 to
2e21ed0
Compare
|
The problem is that now it is inconsistent with all the other Mixin exports we have which do have multiple exports. I've been trying to get rid of most of them though. |
|
By searching for
Here's my suggestion: we update the three that only return a mixin to just return the mixin and don't touch the other ones. I don't think that it's going to leave the codebase in an inconsistent way. |
As mentioned in facebook#7581 (comment) we can remove the Mixin layer of indirection as it only exports a Mixin and I find it confusing.
As mentioned in facebook#7581 (comment) we can remove the Mixin layer of indirection as it only exports a Mixin and I find it confusing.
|
I went ahead and created a PR for
If you think it's a good idea I can merge them all. If not, please let me know how you want me to proceed forward :) |
|
Sounds good. |
As mentioned in #7581 (comment) we can remove the Mixin layer of indirection as it only exports a Mixin and I find it confusing.
As mentioned in #7581 (comment) we can remove the Mixin layer of indirection as it only exports a Mixin and I find it confusing.
As mentioned in #7581 (comment) we can remove the Mixin layer of indirection as it only exports a Mixin and I find it confusing. (cherry picked from commit dd0c65c)
This one is interesting because we have transaction objects being passed around everywhere in the codebase but there's actually no Transaction class. It's a "mixin" that comes to life by being Object.assigned to the prototype of a real "class" (before class was cool!). Therefore, we can't just say `var Transaction = require('Transaction'); (transaction: Transaction) => { }` because it would be the object that contains a mixin and not an instance of a transaction.
The trick I use is to export `TransactionType` and alias it to `Transaction` in the file as it doesn't actually require transaction. In case they do, we'll figure it out, but in the few files I looked at, it doesn't seem to be the case.
For the perform function, it actually typechecks pretty well!
(cherry picked from commit a3e576e)
This one is interesting because we have transaction objects being passed around everywhere in the codebase but there's actually no Transaction class. It's a "mixin" that comes to life by being Object.assigned to the prototype of a real "class" (before class was cool!). Therefore, we can't just say
var Transaction = require('Transaction'); (transaction: Transaction) => { }because it would be the object that contains a mixin and not an instance of a transaction.The trick I use is to export
TransactionTypeand alias it toTransactionin the file as it doesn't actually require transaction. In case they do, we'll figure it out, but in the few files I looked at, it doesn't seem to be the case.For the perform function, it actually typechecks pretty well!