-
Notifications
You must be signed in to change notification settings - Fork 5
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
♻ PoC navigators with set #294
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #294 +/- ##
========================================
- Coverage 100% 99.15% -0.85%
========================================
Files 97 105 +8
Lines 285 356 +71
========================================
+ Hits 285 353 +68
- Misses 0 3 +3
Continue to review full report at Codecov.
|
🎉
|
@frinyvonnick |
|
||
export class ArrayNav extends BaseNav { | ||
get length() { | ||
if (this._length === undefined) this._length = length(this.value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this optimization really efficient ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right, I'll remove this
copy() { | ||
const { value } = this | ||
if (isNil(value)) return [] | ||
return Array.isArray(value) ? [...value] : { ...value } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you testing if this is an Array
whereas we are in ArrayNav
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because users might use slice or array index notation on an object, this is to avoid replacing the object by an array.
This conforms to normal JavaScript behavior:
const obj = {}
obj[0] = 'val'
console.log(obj) // Outputs: {0: "val"}
class IndexNav extends ArrayNav { | ||
constructor(value, index, next) { | ||
super(value, next) | ||
this._index = index |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could use private syntax https://github.com/tc39/proposal-private-methods#private-methods-and-fields ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this is appropriate, let's close our eyes on the ugly #
and try this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find the babel plugin corresponding to the proposal.
I think it's time to rewrite in TS...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK it seems to be in babel 7 beta 48, it might be risky to upgrade to babel 7 but I'll give it a try...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
if (isNil(value)) return [] | ||
|
||
return Array.from(range, index => _next(value[index]).get()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
I'll try upgrading to babel 7 and using private fields in another PR. |
* 🚧 PoC nav * 🚧 Add index navigator * 🚧 Add slice navigator * ♻️ Reorganize code * ♻️ Move apart get and update logic and use ES6 classes * 🔥 Remove unused currification level * ⚡ Use nav in set and validate performance improvement with benchmark * ✨ Put back negative array index support * ✅ Report apply tests to nav * 🚚 Move each navigator in its own file as suggested by @frinyvonnick * :refactor: Optional currying on set * ✨ Add all props navigator * ✨ Prop list navigator * ♻️ Put TypeError for empty path in nav * 👌 @frinyvonnick's review
No description provided.