Fix Bugs/array.prototype.find in v1 master #188
Conversation
hthetiot
commented
Dec 7, 2017
•
hthetiot
commented
- Deprecate find() in favor of findValue()
- Deprecate findLast() in favor of findLastValue()
- Add console warning
- Detect legacy usage and display warning
- Add ES6 Array.prototype.find shim when missing
- Add ES6 Array.prototype.find basic spec
…findLastValue with deprecated warnings
|
Need to add Array.prototype.find when missing also i think. |
…le obj and func before warning
deque.js
Outdated
| typeof console !== 'undefined' && | ||
| typeof console.warn === 'function' | ||
| ) { | ||
| console.warn('.find function is deprecated please use findValue instead.'); |
There was a problem hiding this comment.
Should only leave this note once for the module’s lifetime.
There was a problem hiding this comment.
added deprecatedWarn in each file for now, with TODO remove in v6 notes
deque.js
Outdated
| ) { | ||
| console.warn('.find function is deprecated please use findValue instead.'); | ||
| } | ||
| return Deque.prototype.findValue.apply(this, arguments); |
There was a problem hiding this comment.
Bypassing dynamic dispatch is likely overkill here. If a derivative type overrides find, we should use it, but you’re right to ensure that you forward variadic args in case they extend the API. Consider…
this.find.apply(this, arguments);
There was a problem hiding this comment.
done in Array|Deque|SortedArray
shim-array.js
Outdated
| typeof console !== 'undefined' && | ||
| typeof console.warn === 'function' | ||
| ) { | ||
| console.warn('Overrided Array.prototype.' + key, "Saved into _" + key); |
There was a problem hiding this comment.
It might be worthwhile to make a warn function that isolates the concern of feature detecting console.
There was a problem hiding this comment.
done via deprecatedWarn
shim-array.js
Outdated
| console.warn('Overrided Array.prototype.' + key, "Saved into _" + key); | ||
| } | ||
| define("_" + key, Object.getOwnPropertyDescriptor(Array.prototype, key).value); | ||
| } |
There was a problem hiding this comment.
…but in general I’m not sure what purpose is served by setting aside the punched property. I don’t think anyone can rely on overridden properties being set aside, and in the cases where you care, it will be clearer to do so manually.
There was a problem hiding this comment.
done via ArrayFindPrototype
shim-array.js
Outdated
| @@ -131,8 +142,77 @@ define("deleteAll", function (value, equals) { | |||
| } | |||
| return count; | |||
| }); | |||
| // https://tc39.github.io/ecma262/#sec-array.prototype.find | |||
| if (!Array.prototype.find) { | |||
There was a problem hiding this comment.
Indentation style is not consistent below. Consider unexpand -t 2 | expand -t 4.
shim-array.js
Outdated
|
|
||
| define("find", function (value, equals) { | ||
| // 7. Return undefined. | ||
| return undefined; |
There was a problem hiding this comment.
Unnecessarily explicit, since omitting return entirely does the job.
shim-array.js
Outdated
| define("find", function (value, equals, index) { | ||
| if ( | ||
| typeof arguments[0] === 'function' | ||
| && this instanceof Array |
There was a problem hiding this comment.
Opinion: I prefer to hang all operators off the end of the prior line and align terms of equal precedence on the same column.
if (
x &&
y &&
z
) {
}…l, fix Array.proto.find shim ident
|
Thx @kriskowal for wonderful review. |
