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

fix(traverse): set the correct context when getting multiple NodePath #12848

Closed
wants to merge 3 commits into from

Conversation

fedeci
Copy link
Member

@fedeci fedeci commented Feb 22, 2021

Q                       A
Fixed Issues? Fixes #12570
Patch: Bug Fix? Yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

@babel-bot
Copy link
Collaborator

babel-bot commented Feb 22, 2021

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/47769/

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 22, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit c7db2a6:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@nicolo-ribaudo nicolo-ribaudo added pkg: traverse PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Feb 22, 2021
@nicolo-ribaudo
Copy link
Member

Could you check if this also fixes #12631, which is another context-related bug?

@fedeci
Copy link
Member Author

fedeci commented Feb 22, 2021

I applied the same changes to node_modules/ of the reproduction repo and it didn't work.

return NodePath.get({
listKey: key,
parentPath: this,
parent: node,
container: container,
key: i,
}).setContext(context);
}).setContext(v.context);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think v.context should be supplied as a fallback of context.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the context defaults to true, so if no specific context is passed it will be used the one of the current NodePath (It may not be the right one).
Because of this we can't fallback since context is always defined when calling #get().

function get<T extends t.Node>(
this: NodePath<T>,
key: string,
context: true | TraversalContext = true,
): NodePath | NodePath[] {
if (context === true) context = this.context;
const parts = key.split(".");
if (parts.length === 1) {
// "foo"
return this._getKey(key, context);
} else {
// "foo.bar"
return this._getPattern(parts, context);
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case we can use v.context when context is true, otherwise when a context is indeed passed as TraversalContext, we should respect the input, though.

@@ -206,36 +205,39 @@ export { get };
export function _getKey<T extends t.Node>(
this: NodePath<T>,
key: string,
context?: TraversalContext,
context?: true | TraversalContext,
Copy link
Member Author

@fedeci fedeci Feb 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple questions.
Since this method is only called from #get() this shouldn't be marked as optional because it will default to true when no value is provided. Right?

Also this is not expected to be called from third party plugins, then it shouldn't be exported?

}
}

export function _getPattern(
this: NodePath,
parts: string[],
context?: TraversalContext,
context?: true | TraversalContext,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same for this.

return NodePath.get({
listKey: key,
parentPath: this,
parent: node,
container: container,
key: i,
}).setContext(context);
}).setContext(isValidContext ? context : v.context);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v.context is always undefined because v is a Node not a NodePath.

@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: traverse PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
4 participants