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

Need a way to prepend a node with another node #32

Closed
adamjanicki2 opened this issue Jan 15, 2024 · 12 comments
Closed

Need a way to prepend a node with another node #32

adamjanicki2 opened this issue Jan 15, 2024 · 12 comments

Comments

@adamjanicki2
Copy link
Contributor

adamjanicki2 commented Jan 15, 2024

As mentioned in Mavo #1003, we will need a general way to expand MemberExpressions by prepending, and maybe also appending.

Prepending would look something like:

ast = bar.baz;
ast.prepend(foo);
// ast is now foo.bar.baz

Should this function return void or return the new node without modifying the original? Or should it modify the original and return it? I'm not sure off the top of my head which would be most useful

Would having an append function be useful as well?

@LeaVerou
Copy link
Member

we will need a general way to expand MemberExpressions

Note that this is not specific to MemberExpressions (bar.bazfoo.bar.baz), it could be applied to any type (not always with useful results), e.g.:

  • barfoo.bar
  • bar()foo.bar()
  • 5foo[5]
  • bar + bazfoo[bar + baz]

and so son.
Same with the object, it doesn't necessarily need to be an Identifier, it could be anything, e.g. foo(bar + baz).foo.

ast.prepend(foo);

These are plain objects, so they can't have methods. Perhaps you mean prepend(node, object)?

  • Some other name ideas: qualify(), namespace() (I think I like prepend() better, but a little brainstorming can't hurt).
  • What signature makes the most sense? There's (property, object, options), but we could also explore using a dictionary if the order of arguments is non-obvious (e.g. prepend(bar, { width: foo })). We will likely also need an options dictionary, either from the start or later, so we should plan for that.
  • We likely want to allow string OR node for the object, and if a string is provided, parse it. E.g. the Mavo case then becomes prepend("$fn", root) rather than prepend({ type: "Identifier", name: "$fn" }, root). Remember, if it could be done automatically, it probably should. ;)

Should this function return void or return the new node without modifying the original? Or should it modify the original and return it?

If it returns a new node, what parent pointers would be adjusted? To what?
Note that users can always call map() to clone, so the other option is still possible.

That said, returning the new node would still be useful, otherwise it's nontrivial to get it from the user's pov.

@adamjanicki2
Copy link
Contributor Author

Note that this is not specific to MemberExpressions (bar.bazfoo.bar.baz), it could be applied to any type (not always with useful results), e.g.:

  • barfoo.bar
  • bar()foo.bar()
  • 5foo[5]
  • bar + bazfoo[bar + baz]

and so son. Same with the object, it doesn't necessarily need to be an Identifier, it could be anything, e.g. foo(bar + baz).foo.

Ok, having a general one makes more sense, will do!

ast.prepend(foo);

These are plain objects, so they can't have methods. Perhaps you mean prepend(node, object)?

Yes this is what I meant 😅

  • Some other name ideas: qualify(), namespace() (I think I like prepend() better, but a little brainstorming can't hurt).

I also like prepend the best; we can change our minds and I can rename as I'm working on the PR. Related to your next point, we could also name it something like combine, and then perhaps a function signature of combine(object, property, options) makes the most sense, since the object would be read first when reading the result from left to right. Or at least this makes the most sense to me.

  • What signature makes the most sense? There's (property, object, options), but we could also explore using a dictionary if the order of arguments is non-obvious (e.g. prepend(bar, { width: foo })). We will likely also need an options dictionary, either from the start or later, so we should plan for that.
  • We likely want to allow string OR node for the object, and if a string is provided, parse it. E.g. the Mavo case then becomes prepend("$fn", root) rather than prepend({ type: "Identifier", name: "$fn" }, root). Remember, if it could be done automatically, it probably should. ;)

Good idea was planning on this already!

If it returns a new node, what parent pointers would be adjusted? To what? Note that users can always call map() to clone, so the other option is still possible.

If it returns the new combined node, then the parent pointers of object and property could be readjusted to the new node, something like

...
const combinedNode = ...some calculations...;
parents.set(object, combinedNode, {force: true});
parents.set(property, combinedNode, {force: true});
...

@LeaVerou
Copy link
Member

I also like prepend the best; we can change our minds and I can rename as I'm working on the PR. Related to your next point, we could also name it something like combine, and then perhaps a function signature of combine(object, property, options) makes the most sense, since the object would be read first when reading the result from left to right. Or at least this makes the most sense to me.

I think combine() is too vague, combine how?

But that's what worries me, that it's non-obvious whether the order should be property, object or object, property and people will have to keep looking it up…

If it returns a new node, what parent pointers would be adjusted? To what? Note that users can always call map() to clone, so the other option is still possible.

If it returns the new combined node, then the parent pointers of object and property could be readjusted to the new node, something like

...
const combinedNode = ...some calculations...;
parents.set(object, combinedNode, {force: true});
parents.set(property, combinedNode, {force: true});
...

That’s not enough, you also need to set combinedNode’s parent accordingly and adjust the parent's corresponding child pointer to point to the new node.

@LeaVerou
Copy link
Member

Also, if we use transform() for this, it may make sense to break it down into generating an object literal for the transformation, and a function that applies it in one go (probably 1-2 loc). Then people that want to use map() and have new objects, can simply import the mapping object.

@LeaVerou LeaVerou changed the title Need a way to prepend an object to an existing MemberExpression Need a way to prepend a node with another node Jan 15, 2024
@adamjanicki2
Copy link
Contributor Author

Also, if we use transform() for this, it may make sense to break it down into generating an object literal for the transformation, and a function that applies it in one go (probably 1-2 loc). Then people that want to use map() and have new objects, can simply import the mapping object.

Can you elaborate more on what you mean by using transform here?

@LeaVerou
Copy link
Member

Also, if we use transform() for this, it may make sense to break it down into generating an object literal for the transformation, and a function that applies it in one go (probably 1-2 loc). Then people that want to use map() and have new objects, can simply import the mapping object.

Can you elaborate more on what you mean by using transform here?

It seems natural for the implementation to involve transform()?

@adamjanicki2
Copy link
Contributor Author

It seems natural for the implementation to involve transform()?

If you were implementing this, how would you involve transform in your implementation? Asking because I made a super quick (and currently incorrect due to the parents stuff) attempt at it in #33 and didn't see anywhere where using transform would be useful

@adamjanicki2
Copy link
Contributor Author

Just for clarification, should prepend mutate the node we're prepending onto, or return a new node? Should it do something like transform where it mutates everything except the top-most node but returns the expected result? I'm trying to think about use cases for this function. From an implementation standpoint, it's most natural to return a new node, but I'm unsure about what the most common use cases for this function would be. Any thoughts @LeaVerou? Maybe I am just overcomplicating this and confusing myself 😅

@LeaVerou
Copy link
Member

To properly make the transformation it also needs to modify the child pointers of the parent node so I'm not sure how useful this would be if it didn't mutate.

@adamjanicki2
Copy link
Contributor Author

Ok thanks, this makes sense. I'm clear on mutation of parent/child pointers. Should this function work the same as transform where it returns the result to avoid using Object.assign on the original argument, or should we mutate the original node?

On one hand, doing it the same as transform follows a previously set design pattern and doesn't use Object.assign, which is nice, but on the other hand, as a developer, I would find it a little annoying if this function didn't directly mutate the argument I pass in. For example, I would rather have to write prepend(foo, bar) than bar = prepend(foo, bar). What are your thoughts on this @LeaVerou ? I could be swayed either way, both have benefits and drawbacks.

@LeaVerou
Copy link
Member

I think it's convenient to return the new node so we don't have to find it again, but it should definitely adjust all the pointers needed.

@adamjanicki2
Copy link
Contributor Author

adamjanicki2 commented Jan 23, 2024 via email

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

2 participants