Skip to content

WeakMap.prototype.set should return a TypeError#13059

Closed
jasonmit wants to merge 1 commit intoemberjs:masterfrom
jasonmit:patch-3
Closed

WeakMap.prototype.set should return a TypeError#13059
jasonmit wants to merge 1 commit intoemberjs:masterfrom
jasonmit:patch-3

Conversation

@jasonmit
Copy link
Member

@jasonmit jasonmit commented Mar 6, 2016

@mixonic
Copy link
Member

mixonic commented Mar 6, 2016

@jasonmit I'm unsure is this is a tradeoff between spec adherence vs. performance. /cc @stefanpenner

@stefanpenner
Copy link
Member

Ya I'm curious as to the performance impact, if we do decide to keep the assertion around in prod, the following may be the most performant variant. (largely focusing on keeping AST size to a min, so allow for more inlining).

var type = typeof obj;
if (obj && (type === 'object' || type === 'function')) {
   metaFor(obj).writableWeak()[this._id] = (value === undefined ? UNDEFINED : value);
   return this;
}

invalidKey()

Obviously this may just be pre-mature optimization, but I am genuinely curious as to the impact. https://github.com/stefanpenner/do-you-even-bench may be used to quickly test the differences

My guess is metaFor(obj).writableWeak()[this._id] will overshadow the costs of the assertion anyways..

@homu
Copy link
Contributor

homu commented Jun 15, 2016

☔ The latest upstream changes (presumably #13658) made this pull request unmergeable. Please resolve the merge conflicts.

@stefanpenner
Copy link
Member

cc @jasonmit

@jasonmit jasonmit closed this Jun 20, 2016
@jasonmit jasonmit deleted the patch-3 branch June 20, 2016 01:27
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

Successfully merging this pull request may close these issues.

4 participants