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

Detect awaited rhs values in top level declarations #6

Closed
wants to merge 5 commits into from

Conversation

perbergland
Copy link

Now detects TLA for:

const x = await functionCall();

Includes changes in #5

@perbergland
Copy link
Author

I’ve never written code for Babel visitors before so feel free to suggest improvements.

@StorytellerCZ StorytellerCZ requested a review from zodern May 4, 2024 22:02
@zodern
Copy link
Collaborator

zodern commented May 6, 2024

This looks good for the specific issue you found. However, it seems this is a much more general issue - there's many other situations where it misses top level await. A couple examples are:

if (true) {
   var a = await 0; 
}

console.log(count(await 5));

What I think we need to do is have visitAwaitExpression walk up the parent nodes and find the first function node or the Program node to see if it is a top level await. A list of the node types for functions is here.

Another option might be to have the visitor visit each of the function nodes, and track if we are inside a function or not to avoid walking up the parents. I'm not sure if this is more efficient, but it seems it could be significantly faster.

@perbergland
Copy link
Author

@zodern Yes that thought occurred to me too but would you mind approving this PR as a step in the right direction?

@perbergland
Copy link
Author

ping @zodern pls approve for this incremental improvement that should cover a lot of typical usages of TLA

@zodern
Copy link
Collaborator

zodern commented May 13, 2024

It looks good to me. I'll leave it up to Meteor if they want to temporarily use this before fully fixing the issue.

@leonardoventurini
Copy link
Member

This PR should cover this one and also cover a wider range of cases

#8

cc @zodern @perbergland

@perbergland
Copy link
Author

Brilliant!

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.

3 participants