Skip to content

Type PooledClass #7578

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

Merged
vjeux merged 1 commit into from
Aug 28, 2016
Merged

Type PooledClass #7578

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
5 changes: 2 additions & 3 deletions src/renderers/shared/utils/CallbackQueue.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
* of patent rights can be found in the PATENTS file in the same directory.
*
* @providesModule CallbackQueue
* @flow
*/

'use strict';
Expand Down Expand Up @@ -101,6 +102,4 @@ Object.assign(CallbackQueue.prototype, {

});

PooledClass.addPoolingTo(CallbackQueue);

module.exports = CallbackQueue;
module.exports = PooledClass.addPoolingTo(CallbackQueue);
Copy link
Copy Markdown
Collaborator

@gaearon gaearon Aug 28, 2016

Choose a reason for hiding this comment

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

Do we need to do it in a single statement here? I’m asking because usually we write module.exports = ThingWithSameNameAsFile and the inconsistency is slightly jarring.

Copy link
Copy Markdown
Contributor Author

@vjeux vjeux Aug 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I'm afraid that doing

CallbackQueue = PooledClass.addPoolingTo(CallbackQueue);

is going to confuse flow in the file (I haven't tried).

If creating a new variable, I'm not sure how to call it, maybe

var PooledCallbackQueue = PooledClass.addPoolingTo(CallbackQueue);
module.exports = PooledCallbackQueue;

which is also inconsistent with the ThingWithSameNameAsFile rule.

There's precedent with this in the Relay world where you first declare your React component and then do

module.exports = Relay.createContainer(ReactComponent, { ... graphql query ... });

People are also doing this with Redux

module.exports = connect(...

I'm happy to change it to something else :)

Copy link
Copy Markdown
Collaborator

@gaearon gaearon Aug 28, 2016

Choose a reason for hiding this comment

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

Yea, I just meant in this particular codebase.
Not feeling strongly about it.

Copy link
Copy Markdown
Contributor Author

@vjeux vjeux Aug 28, 2016

Choose a reason for hiding this comment

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

I'm more than happy to change it to something else, I'm just not sure what this something else is.

25 changes: 18 additions & 7 deletions src/shared/utils/PooledClass.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
* of patent rights can be found in the PATENTS file in the same directory.
*
* @providesModule PooledClass
* @flow
*/

'use strict';
Expand Down Expand Up @@ -90,6 +91,8 @@ var standardReleaser = function(instance) {
var DEFAULT_POOL_SIZE = 10;
var DEFAULT_POOLER = oneArgumentPooler;

type Pooler = any;

/**
* Augments `CopyConstructor` to be a poolable class, augmenting only the class
* itself (statically) not adding any prototypical fields. Any CopyConstructor
Expand All @@ -99,8 +102,16 @@ var DEFAULT_POOLER = oneArgumentPooler;
* @param {Function} CopyConstructor Constructor that can be used to reset.
* @param {Function} pooler Customizable pooler.
*/
var addPoolingTo = function(CopyConstructor, pooler) {
var NewKlass = CopyConstructor;
var addPoolingTo = function<T>(
CopyConstructor: Class<T>,
pooler: Pooler,
): Class<T> & {
getPooled(/* arguments of the constructor */): T;
release(): void;
} {
// Casting as any so that flow ignores the actual implementation and trusts
// it to match the type we declared
var NewKlass = (CopyConstructor: any);
NewKlass.instancePool = [];
NewKlass.getPooled = pooler || DEFAULT_POOLER;
if (!NewKlass.poolSize) {
Expand All @@ -112,11 +123,11 @@ var addPoolingTo = function(CopyConstructor, pooler) {

var PooledClass = {
addPoolingTo: addPoolingTo,
oneArgumentPooler: oneArgumentPooler,
twoArgumentPooler: twoArgumentPooler,
threeArgumentPooler: threeArgumentPooler,
fourArgumentPooler: fourArgumentPooler,
fiveArgumentPooler: fiveArgumentPooler,
oneArgumentPooler: (oneArgumentPooler: Pooler),
twoArgumentPooler: (twoArgumentPooler: Pooler),
threeArgumentPooler: (threeArgumentPooler: Pooler),
fourArgumentPooler: (fourArgumentPooler: Pooler),
fiveArgumentPooler: (fiveArgumentPooler: Pooler),
};

module.exports = PooledClass;