Skip to content

Conversation

brunocroh
Copy link
Member

When a module is imported with property access after require

const bind = require("module").a.b.c
const result = resolveBindingPath(node, "$.a.b.c")

It resolve as bind.a.b.c However, since the property is accessed immediately after require, it should instead resolve as bind, because the value of c is directly assigned to the bind constant.

This changes fixes that behavior.

@brunocroh brunocroh requested a review from a team September 28, 2025 21:02
Copy link
Member

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

is it possible to have handling something like :

import foo from 'node:foo'

foo.bar.functionToModify();

@brunocroh
Copy link
Member Author

is it possible to have handling something like :

import foo from 'node:foo'

foo.bar.functionToModify();

I think this test already covers this scenario, doesn’t it?

it("should be able to solve binding path when destructuring happen", () => {
const code = dedent`
const { types } = require('node:util');
`;
const rootNode = astGrep.parse(astGrep.Lang.JavaScript, code);
const importStatement = rootNode.root().find({
rule: {
kind: "variable_declarator",
},
});
const bindingPath = resolveBindingPath(importStatement!, "$.types.isNativeError");
assert.strictEqual(bindingPath, "types.isNativeError");
});

@AugustinMauroy
Copy link
Member

is it possible to have handling something like :

import foo from 'node:foo'

foo.bar.functionToModify();

I think this test already covers this scenario, doesn’t it?

it("should be able to solve binding path when destructuring happen", () => {
const code = dedent`
const { types } = require('node:util');
`;
const rootNode = astGrep.parse(astGrep.Lang.JavaScript, code);
const importStatement = rootNode.root().find({
rule: {
kind: "variable_declarator",
},
});
const bindingPath = resolveBindingPath(importStatement!, "$.types.isNativeError");
assert.strictEqual(bindingPath, "types.isNativeError");
});

Yeah but for esm

@brunocroh
Copy link
Member Author

is it possible to have handling something like :

import foo from 'node:foo'

foo.bar.functionToModify();

Done

Copy link
Member

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

LGMT ! Thanks bruno

@AugustinMauroy AugustinMauroy requested a review from a team October 1, 2025 10:19
@AugustinMauroy AugustinMauroy added the awaiting reviewer Author has responded and needs action from the reviewer label Oct 1, 2025

if (propertyAccesses.length) {
const pathArr = path.split(".");
let newPath = ["$"];
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think newPath is actually constant?

Comment on lines +83 to +85
let i = 0;

for (; i < propertyAccesses.length; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

i will actually be scoped to the parent of for automatically, so extracting it like this is unnecessary, but perhaps you did it to make it more obvious that the parent scoping is necessary/intended?

@JakobJingleheimer JakobJingleheimer added awaiting author Reviewer has requested something from the author and removed awaiting reviewer Author has responded and needs action from the reviewer labels Oct 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting author Reviewer has requested something from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants