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

v2.0.2 add 'add' function to array's prototype #551

Open
Alexsey opened this issue Jul 7, 2014 · 11 comments
Open

v2.0.2 add 'add' function to array's prototype #551

Alexsey opened this issue Jul 7, 2014 · 11 comments

Comments

@Alexsey
Copy link

Alexsey commented Jul 7, 2014

console.log([].add && [].add.toString())
var q = require('q')
console.log([].add && [].add.toString())

with 2.0.2

undefined
function (value) {
this.push(value);
return true;
}

with 1.0.1

undefined
undefined

@Alexsey Alexsey changed the title v2.0.2 add 'add' function to array prototype v2.0.2 add 'add' function to array's prototype Jul 7, 2014
@anilanar
Copy link

It comes from the collections dependency added in commit: 6c39534
See ./node_modules/collections/shim_array.js:

define("add", function (value) {
  this.push(value);
  return true;
});

I'm not sure if it's intended.

@kriskowal
Copy link
Owner

@Alexsey can you describe how this interferes with your application? The Collections package does shim some methods on Array.

@Alexsey
Copy link
Author

Alexsey commented Aug 14, 2014

We use sugarjs in our project, it's a library that brings a lot of methods to prototypes, and it also has .add method. So after Q installation code just break. Single project should not have more than one library that extends prototypes, and imho it should do library that extends prototypes, not one that bring promises.

@domenic
Copy link
Collaborator

domenic commented Aug 14, 2014

not one that bring promises.

+1; this will be a dealbreaker for using Q in many shops.

Are we using more of collections than is provided by the WeakMap shim?

@kriskowal
Copy link
Owner

I’m bringing in the iterator shim from collections@future for remote streaming. I will need to find a middle road before q@future is prime time.

@Alexsey Please note that this is not a problem for the official release of Q, and Q version 2 is not recommended for anything but experimental use.

@kriskowal
Copy link
Owner

The most viable solution is to remove shimming in Collections and instead expose operator modules. cc @Stuk

@Stuk
Copy link

Stuk commented Aug 14, 2014

Could you be more explicit about what you mean by "operator modules"?

@kriskowal
Copy link
Owner

@Stuk, In Python at least, the strategy for adding operators to lower level objects and allowing higher level objects to override is to expose an operator like len(object) that implements the behavior for lower level objects (checking the length of a builtin like list and set) and then if the type is unknown, delegate to a method if it exists, like object.__len__().

Collections provides equals, compare, and iterate operators.
These could conceivably be exposed the Python way instead of as Object.equals, Object.compare, and Array.prototype.iterate().
The usage would become:

var iterate = require("collections/iterate");
iterate(collection);

For now, the iterate operator would still have to delegate to collection.iterate() as a named method, but ES-next introduces Symbol.iterate. In keeping with that style, operator modules would probably export a symbol property on the operator, to allow higher level objects to safely add overrides for any operator module.

var iterate = require("collections/iterate");
MySet.prototype[iterate.symbol] = function () {
};

@Alexsey
Copy link
Author

Alexsey commented Aug 14, 2014

@kriskowal yes, of course it's not a problem of official release, that is why 2.0.2 is in the title.

@kriskowal
Copy link
Owner

Thanks, @Alexsey. I worry about breaking people’s stuff with my moon shot branch.

@Alexsey
Copy link
Author

Alexsey commented Aug 14, 2014

@kriskowal, thanks for reply. Hope I would be able to close this soon :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants