Skip to content

Conversation

@brunocroh
Copy link
Member

@brunocroh brunocroh commented Aug 25, 2025

Closes #187

Add support to getNodeImportCalls returns the entire block when found a dynamic import with promise.

like the block below

await import("node:fs").then((mdl) => {
	const readfile = mdl.readFile;
	readfile("foo.txt", "utf8", (err, data) => {
		if (err) throw err;
		console.log(data);
	});
});

This utility function can now return two different types of nodes:

kind: variable_declarator
Indicates that a variable was created in the file scope.

kind: expression_statement
Indicates that it is a dynamic import, and the imported module likely exists only within the scope of the then function.


Support handling imports that use variables as well.

const moduleName= "node:fs";

const fs = await import(moduleName)

Copy link
Member

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

Cool, thanks!


Will this handle bizarre things like

const dep = import('node:util');

const { default: util } = await dep;
function isNodeModule(specifier) { /* … */ }

await isNodeModule('node:util');

P.S. Let's avoid large formatting changes mixed into unrelated PRs 🙏 It bloats the diff making it more difficult to figure out what changes actually matter.

@brunocroh
Copy link
Member Author

P.S. Let's avoid large formatting changes mixed into unrelated PRs 🙏 It bloats the diff making it more difficult to figure out what changes actually matter.

Noted, sorry about it

Will this handle bizarre things like

const dep = import('node:util');

const { default: util } = await dep;

No, this scenario will not be handled with this implementation. To start covering it, we would need to update the function interface, because in this case we need to return more than one SgNode for a single import case. The current return type does not support this, so it would introduce a breaking change.

So I would like to await a little bit, before does this refactoring, for two reasons.

  1. It’s a very specific edge case. I think it’s fine not to cover it for a while and maybe wait for some user feedback.

  2. I’d like to see more use cases and examples of needs in our recipes before we make breaking changes in utils package. This would help us avoid over-engineering the and reduce the risk of introducing another breaking change soon.

function isNodeModule(specifier) { /* … */ }

await isNodeModule('node:util');

Sorry but I didn't get what is happening here, and what is the expected output, could please you give more details?

@JakobJingleheimer
Copy link
Member

No, this scenario will not be handled with this implementation. To start covering it, we would need to update the function interface, because in this case we need to return more than one SgNode for a single import case. The current return type does not support this, so it would introduce a breaking change.

So I would like to await a little bit, before does this refactoring, for two reasons.

That sounds reasonable to me. Perhaps if we land it now, mark the version number as some kind of pre-v1 then to signify that we know it's incomplete (and maybe add that known missing edge-case to the README)? I could maybe be convinced for 1.0.0 with a subsequent patch, but I currently think the pre v1 is the way to go.


function isNodeModule(specifier) { /* … */ }

await isNodeModule('node:util');

Sorry but I didn't get what is happening here, and what is the expected output, could please you give more details?

This is a case where an unrelated function is called with an argument needle we're searching for. If the search criteria is "a function called with node:…", this would included.

@JakobJingleheimer JakobJingleheimer added the awaiting author Reviewer has requested something from the author label Sep 17, 2025
@brunocroh
Copy link
Member Author

No, this scenario will not be handled with this implementation. To start covering it, we would need to update the function interface, because in this case we need to return more than one SgNode for a single import case. The current return type does not support this, so it would introduce a breaking change.
So I would like to await a little bit, before does this refactoring, for two reasons.

That sounds reasonable to me. Perhaps if we land it now, mark the version number as some kind of pre-v1 then to signify that we know it's incomplete (and maybe add that known missing edge-case to the README)? I could maybe be convinced for 1.0.0 with a subsequent patch, but I currently think the pre v1 is the way to go.

function isNodeModule(specifier) { /* … */ }

await isNodeModule('node:util');

Sorry but I didn't get what is happening here, and what is the expected output, could please you give more details?

This is a case where an unrelated function is called with an argument needle we're searching for. If the search criteria is "a function called with node:…", this would included.

Sorry for delay, All done =)

Copy link
Member

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

LGMT. If it could set the version to the pre-release thing, and note somewhere that it does not handle the bizarre non-awaited dynamic import, g2g.

@JakobJingleheimer JakobJingleheimer removed the awaiting author Reviewer has requested something from the author label Sep 22, 2025
@brunocroh
Copy link
Member Author

LGMT. If it could set the version to the pre-release thing, and note somewhere that it does not handle the bizarre non-awaited dynamic import, g2g.

Actually, our package @nodejs/codemod-utils is at version 0.0.0. Which version should I update it to 0.0.1?

@JakobJingleheimer
Copy link
Member

Ah, it's the consumers that would need to be pre-release (and then they could all be bumped once this edge-case is covered).

@brunocroh brunocroh merged commit a0386d1 into nodejs:main Sep 22, 2025
@brunocroh brunocroh deleted the feature/import-then branch September 22, 2025 16:01
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

Successfully merging this pull request may close these issues.

feat(utility): extend getNodeImportCalls to support then methods

3 participants