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

fix TLA with esnext syntax #33

Merged
merged 4 commits into from
Aug 29, 2019
Merged

fix TLA with esnext syntax #33

merged 4 commits into from
Aug 29, 2019

Conversation

caub
Copy link
Contributor

@caub caub commented Aug 23, 2019

currently n> class A{x=1} await 1 fails, because acorn doesn't handle esnext syntax, I could have used npm.im/acorn-stage3, but acorn-walk doesn't work with those stage3 new nodes, so I went full babel

@caub caub force-pushed the fix-tla branch 2 times, most recently from 4321145 to d7f048e Compare August 23, 2019 13:16
@ljharb
Copy link
Contributor

ljharb commented Aug 23, 2019

The entire point of n> is to use no babel and match node exactly.

@ljharb
Copy link
Contributor

ljharb commented Aug 23, 2019

TLA shouldn’t even work in n> until it’s shipped in node.

@caub
Copy link
Contributor Author

caub commented Aug 23, 2019

I see your point, but we are talking about a bot CLI, which has to be practical, just like devtools, or node --experimental-repl-await similarly have this, and other convenience helpers, it doesn't intend to be a real TLA at all

@ljharb
Copy link
Contributor

ljharb commented Aug 23, 2019

Fair. I still think it’s best to avoid Babel for this mode.

@brigand
Copy link
Owner

brigand commented Aug 23, 2019

I think it's fine to support TLA, but only if we avoid running code that doesn't use TLA through babel.

Base babel settings: all syntax plugins enabled

ON_RECEIVE_MESSAGE

  1. Set toExecute to the provided code
  2. search for /\bawait\b/ in toExecute. If not found, go to NODE
  3. attempt to parse with babel, producing an AST. If failed, go to NODE
  4. attempt to find a TLA, and if not found go to NODE
  5. do the babel transform for TLA, if successful then set toExecute to the resulting code
  6. go to NODE

NODE:

  1. Execute toExecute as you normally would with the n> command.

@caub
Copy link
Contributor Author

caub commented Aug 23, 2019

to be clear, I switched from acorn to babel-parser and enabled all syntax plugins (but you're right we don't need all) because I wanted to be able to parse all currently valid code for n> like class A { x = 1; },

current node (12.9.0) supports those:

    'bigInt',
    'classProperties',
    'classPrivateProperties',
    'classPrivateMethods',
    'numericSeparator',
    'optionalCatchBinding',

Enabling all of them doesn't mean n> can suddenly evaluate optional-chaining etc.., no it'll just allow to parse it, and processTLA then stringify it and pass it to the run.js script with mode 'n' where optional-chaining would fail

  • input: await 1
  • processTopLevelAwait transforms in: (async() => { return await 1 })()
  • run.js evaluates (async() => { return await 1 })() with given mode ('b', 'n', 'e')

In case of b> babel mode, we're sort of parsing twice the same code, is it what you want to improve @brigand?

note: I'd really want to avoid using regex for code, it'll fail for some cases let s = "test await 1"

hmm I guess the answer is yes to my previous question @brigand , we can just remove step2, and use babel-parser to detect if the AwaitExpression exists at the top level, I'll work on that

@brigand
Copy link
Owner

brigand commented Aug 23, 2019

In the steps I outlined, the 'await' regex is just to quickly skip the following steps for code that we are sure doesn't have a top level await.

@caub caub force-pushed the fix-tla branch 2 times, most recently from f6d5a77 to 0f4c2cf Compare August 23, 2019 21:37
@caub
Copy link
Contributor Author

caub commented Aug 23, 2019

@brigand Ok done in the last commit, in comparison this is a simpler approach caub/jellobot@fix-tla...caub:fix-tla2 but that parse twice if babel+TLA, so let's use your idea, thanks

src/plugins/js-eval/processTopLevelAwait.js Outdated Show resolved Hide resolved
src/plugins/js-eval/jsEvalPlugin.js Show resolved Hide resolved
brigand added a commit that referenced this pull request Aug 29, 2019
fix TLA with esnext syntax
@brigand brigand merged commit 216bf9b into brigand:master Aug 29, 2019
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