search

meteor / reify

Enable ECMAScript 2015 modules in Node today. No caveats. Full stop.
MIT License
6 stars 3 forks source link

Detect awaited rhs values in top level declarations #6

Closed perbergland closed 4 months ago

perbergland commented 5 months ago

Now detects TLA for:

const x = await functionCall();

Includes changes in #5

perbergland commented 5 months ago

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

zodern commented 5 months ago

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 commented 5 months ago

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

perbergland commented 5 months ago

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

zodern commented 5 months ago

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 commented 5 months ago

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

https://github.com/meteor/reify/pull/8

cc @zodern @perbergland

perbergland commented 5 months ago

Brilliant!