-
-
Notifications
You must be signed in to change notification settings - Fork 19
feat(util/update-binding): add new utility to update binding in the file #182
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
base: main
Are you sure you want to change the base?
Conversation
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.
LGTM ! thanks for that
2b2978d
to
164d78c
Compare
wait these function can support dynamic import |
First, we need to merge #189. Once that is merged, we should update This utility function is called with the result of const importStatement = getNodeImportCalls(node, "util")
const binding = resolveBindingPath(importStatement, "$.diff")
const edits = updateBinding(importStatement, binding, {newBinding: "newDiff"}) |
@brunocroh this is no-longer blocked, right? |
Yep, no more blockers. I'm just finishing a PR for #191, and then I'll come back to this one. |
@brunocroh could you please undo the formatting changes? It's hugely bloating the recent diff |
Done, Jakob. This one is now ready to be reviewed/merged. I’m also covering the scenarios handled in #193. before // Process transformations in order:
// 1. CommonJS require statements
processStatements(root, edits, "require");
// 2. ESM import statements
processStatements(root, edits, "import-static");
// 3. Dynamic import statements
processStatements(root, edits, "import-dynamic"); after const binding = resolveBindingPath(node, "SlowBuffer");
updateBinding(node, {
old: binding,
new: "Buffer"
}) |
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.
what if wee need to change the import itself.
for example
import aFonction from 'node:bar';
have to be updated to import anOtherFunction from 'node:quez';
?
options?: UpdateBindingOptions, | ||
): UpdateBindingReturnType { | ||
const nodeKind = node.kind().toString(); | ||
|
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.
if (kind !=== "") {throws error ...}
we can add check to ensure good node is passed.
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 do not have sure if I get it, some lines below we check the node kind already, I think if this is a kind that we not handle here, it just will be return undefined at the end, could you please give me more context?
if (requireKinds.includes(nodeKind)) {
return handleNamedRequireBindings(node, options);
}
if (importKinds.includes(nodeKind)) {
return handleNamedImportBindings(node, options);
}
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.
why not:
if (requireKinds.includes(nodeKind)) {
return handleNamedRequireBindings(node, options);
} else if (importKinds.includes(nodeKind)) {
return handleNamedImportBindings(node, options);
} else {
throws new Erro(`Invalid node kind [${nodeKind}]`)
}
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 today already returns undefined if not possible update, we throw an error it forces to who is using this utility to put a try/catch on it
Example
actual code
const result = updateBinding(invalidNode, {old, new: "test"})
if (result != undefined) {
// do something
}
What it forces to be
let isPossibleUpdate = true
try {
const result = updateBinding(invalidNode, {old, new: "test"})
// result is undefined so update var to false
if (result === undefined) {
isPossibleUpdate = false
}
}catch(e) {
// error happen, so update var to false
isPossibleUpdate = false
}
// now after if we can write the logic without need to duplicated it
if (isPossibleUpdate) {
}
let me know what 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.
what if invalid node is pass as arg it's an error from dev so if we throws a message it's will allow us to have better DX.
also if there are any change we should return null so we know everything work but nothing happened
Co-authored-by: Augustin Mauroy <[email protected]>
Co-authored-by: Augustin Mauroy <[email protected]>
Add new updateBinding utility function and make removeBinding a wrapper for it
Summary
This PR create the new
updateBinding
function. That can be used to update bindings in the file##Examples
Updating a named require/import
updateBinding(node, 'types', {newBinding: 'newTypes'}
Before:
After:
Removing a named require/import
updateBinding(node, 'types', {newBinding: undefined}
Before:
After:
Updating a named import with alias
updateBinding(node, 'renamedTypes', {newBinding: 'newTypes'}
Before:
After:
Changes Made
updateBinding
function in/utils/src/ast-grep/update-binding.ts
remove-binding
function to use the newupdateBinding
internally