-
Notifications
You must be signed in to change notification settings - Fork 31
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
feat: add traverse
function
#59
Conversation
src/object/traverse.ts
Outdated
export function traverse( | ||
root: object, | ||
visitor: TraverseVisitor, | ||
outerContext?: TraverseContext | null, |
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.
Do we want to make outerContext
a public API? Do you have any nice use case examples for that?
I think it's better to create internalTaverse
which is not exported and export traverse
as a root call of internalTraverse
. It helps us to keep API simple. And we can always add outerContext
argument in the future if we have nice use case for that.
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.
It's intentional. You can call traverse
from within another traverse
visitor if you need to traverse a class instance or a non-array iterable. By passing the context along, you maintain the context.parents
and context.path
properties.
export type TraverseVisitor = ( | ||
value: unknown, | ||
key: keyof any, | ||
parent: object, |
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.
We already have parent inside context, so we can skip that argument at all:
_.traverse(obj, (value, key, {parent}) => {
})
For me, that API looks nicer. What do you think?
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.
This was intentional. The idea is to support any function that implements a “standard” enumeration signature. By matching the signature of Array.prototype.forEach
callbacks for example, we leave open opportunities for code reuse.
* the `visitor` callback. If that's necessary, you'll want to clone | ||
* it first. | ||
*/ | ||
readonly parents: readonly object[] |
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 have a hacky solution for this problem. We can use getter for this get parents
(or maybe method getParents
would be even better (explicit)?). And we can copy array inside getter (so copying is lazy). To prevent a copy of stale array (outside visitor
callback) we can keep iterationIdx
in the context and call iterationIdx++
each time we do recursive call. The code can look something like:
const currentRunIdx = iterationIdx;
const context = {
...outerContext
get parents() {
if (currentRunIdx !== iterationIdx) {
throw Error("traverse context was accessed outside of the callback")
}
return [...parents];
}
}
The good thing is that we don't shadow the error here. But with such API we always copy an array if user access it, which is not performant.
In the end, I don't know how to solve it better.
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.
The getter isn't a bad idea, but it means we'd have to define another undocumented property for the mutable array. I believe the current behavior is well-documented enough and misuse is noticeable enough, so it should be quick/easy for developers to debug their misuse without needing to complicate traverse
further.
d3563ba
to
b9503e7
Compare
2154f96
to
6a4b4f6
Compare
Tip
The owner of this PR can publish a preview release by commenting
/publish
in this PR. Afterwards, anyone can try it out by runningpnpm add radashi@pr<PR_NUMBER>
.Summary
The
traverse
function helps you iterate the properties of an object or the values of an iterable, while also iterating any nested plain objects and arrays deep within.Terminology
traverse
that receives each value found in the root object or an object nested within the root object.Here's a (hopefully) exhaustive list of its behavior:
root
argument of traverse is always traversed. If it's an iterable (e.g. a Map or a Set), then it's iterated with afor..of
loop. If it's an array, it's iterated with.forEach
(so that sparse arrays won't have their holes visited). Otherwise, the object properties are iterated.string
keys get iterated. That's because we useObject.keys
by default, but the caller can provide their own “get object keys” function (the 4th argument). For example, you may passReflect.ownKeys
to iterate all properties whether they're enumerable/non-enumerable or have a string/number/symbol key.(value, key, parent, context)
. Thecontext
argument contains apath
array (the keys used to reach this value) and aparents
array (the chain of objects used to reach this value), along with some others.value
is a traversable object, just callcontext.skip()
. Theskip()
method also accepts an optional object argument, which allows skipping a nested object before it's been visited.traverse
call returns false. Returnfalse
from your visitor callback to end traversal immediately (no more values will be visited).traverse
within your visitor callback. To preserve thecontext.path
andcontext.parents
arrays in your nestedtraverse
call, be sure to pass in thecontext
from the parent traversal.Note: We'll likely use
traverse
to implement deep-cloning in Radashi.Naming
While
traverse
is a decent name, I'm open to other suggestions. Other common names for this type of function include:walk
deepForEach
depthFirstScan
What kind of change does this PR introduce?
Feature
For any code change,
Does this PR introduce a breaking change?
No
Bundle impact
src/object/traverse.ts
src/typed/isIterable.ts