Skip to content

Conversation

@raj-kumar-s-dev
Copy link
Contributor

@vault-developer
I know i disabled the eslint rule in BinaryExpression.ts

It wasn't my intention to do so.
I tried casting the type to BinaryExpression.
But it didn't work and led to more eslint errors.

If you know of a better approach, please let me know.

Copy link
Owner

@vault-developer vault-developer left a comment

Choose a reason for hiding this comment

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

Hey @raj-kumar-s-dev thank you for the PR.
Could you check comments please?

code: `console.log(1);
setTimeout(() => console.log(2), 0);
queueMicrotask(() => console.log(3));
console.log(2-1);
Copy link
Owner

@vault-developer vault-developer Nov 24, 2024

Choose a reason for hiding this comment

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

🎯 suggestion:
Adding 5 additional binary expression examples to the "microtasks" example may create confusion as they are not related to microtasks.
What do you think about adding some of them to "everything" instead?

const operator = binaryExpression.operator;
switch (operator) {
case '+':
return left + right;
Copy link
Owner

Choose a reason for hiding this comment

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

🔨 issue(blocking):
There is no guarantee that binaryExpression left and right value is a number.
For example, this expression will send NaN to console:
console.log(2+(8+3));

image


serialize = () => {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const binaryExpression = this.node as any;
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
const binaryExpression = this.node as any;
import {BinaryExpression} from "acorn";
const binaryExpression = this.node as BinaryExpression;

const left = binaryExpression.left.value;
const right = binaryExpression.right.value;
const operator = binaryExpression.operator;
switch (operator) {
Copy link
Owner

@vault-developer vault-developer Nov 24, 2024

Choose a reason for hiding this comment

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

🎯 suggestion:
I suggest to handle all possible operators, you can check types in acorn:
export type BinaryOperator = "==" | "!=" | "===" | "!==" | "<" | "<=" | ">" | ">=" | "<<" | ">>" | ">>>" | "+" | "-" | "*" | "/" | "%" | "|" | "^" | "&" | "in" | "instanceof" | "**"

}
};

traverse = () => {};
Copy link
Owner

Choose a reason for hiding this comment

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

🎯 suggestion(blocking):
I suggest to traverse binaryExpression members further.
Otherwise, they won't be properly managed by the event loop.
Example:
console.log(1 + console.log(1));

@vault-developer
Copy link
Owner

I'm closing the PR since there have been no updates for more than 1 month.
Feel free to reopen if needed 👍

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.

2 participants