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

WIP DRAFT: Upgrade to luaparse 0.3 #19

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

matthargett
Copy link
Contributor

Not at simple as jumping to prettier 2.1 ;)

These are the comments from the commits below for some context on where this PR is currently at:

luaparse 3.0 now requires you to set an encodingMode, or it defaults to 'none'. The none options strips raw strings, which we use for various purposes, so that won't work. We also have some test cases that need wide character support, which luaparse had an encodingMode for and then removed. Finding the right balance here will require an upstream change+publish in luaparse before this set of changes can be merged, unless we want to modify/disable the tests we have that include wide character strings.

prettier 2.x allows a configuration for specifying if semicolons are required. The new luaparse AST granularity (again, ultimately a step in a good direction) introduces some wrinkles to our current naive approach on adding semicolons. Our test suite isn't totally consistent in what it expects, but it looked like 'no semis' was the majority of the cases. Putting a stake in the ground here for discussion.

luaparse 3.0 produces a more granular AST, which is ultimately a good thing. this is not quite compatible with the assumptions we were able to get away with, so there are a few more mis-transformations that happen in both our test suite and the codebase I test on (Roact). I started using the plugin-php structure for incrementally refining what we have, including aligning to their language-agnostic utility function names. This is not complete, even for just getting the semicolons test suite to pass. After trying things for a few hours, I think a unit-level TDD approach to the utility functions is how I would proceed before trying to move forward with integration-level tests.

Next steps I propose:
-Focus on the semicolon test suite cases, with new jest tests for the relevant utility functions that are based on the php (or other prettier language) plugin utility function names.

-Once the adding semicolon test cases are working (we don't seem to have functionality for removing them, so save that for a separate PR), move onto the unnecessary-parens test cases.

-Once the unnecessary-parens test cases pass, figure out what to do with wide character string support. This may require submitting a PR to luaparse itself.

-Run the whole test suite, do whatever fixups (to tests or implementations) for regressions.

-Run prettier in Roact code base, fix any major regressions.

-Merge/publish.

…onfig-unobtrusive gets in the way of the other alignment.
…to 'none'. The none options strips raw strings, which we use for various purposes, so that won't work. We also have some test cases that need wide character support, which luaparse *had* an encodingMode for and then removed. Finding the right balance here will require an upstream change+publish in luaparse before this set of changes can be merged, unless we want to modify/disable the tests we have that include wide character strings.
…required. Our test suite isn't totally consistent in what it expects, but it looked like 'no semis' was the majority of the cases. Putting a stake in the ground here for discussion.
… thing. this is not quite compatible with the assumptions we were able to get away with, so there are a few more mistransformations that happen in both our test suite and the codebase I test on (Roact). I started using the plugin-php structure for incrementally refining what we have, including aligning to their language-agnostic utility function names. This is *not* complete, even for just getting the semicolons test suite to pass. After trying things for a few hours, I think a unit-level TDD approach to the utility functions is how I would proceed before trying to move forward with integration-level tests.
@matthargett
Copy link
Contributor Author

Most of the remaining regressions are explained by this upstream issue in luaparse:
fstirlitz/luaparse#79

We could work around this by inserting the inParens field to true where needed to emulate the old way it worked. I think it's probably better to fix the upstream problem mentioned in the issue, since that aligns better to the estree AST spec and allows us to plug in any other Lua AST library that complies with the same spec.

@suchipi
Copy link
Member

suchipi commented Sep 15, 2020

Food for thought:

  • I don't often see semicolons in lua code (but I haven't done any roblox stuff). If they aren't common, I don't think it's super important that we support the semi option.
  • What are the benefits of upgrading to luaparse 0.3? I think we might want to wait until luaparse fixes its upstream issues with parens and wide character support before moving to it.

@matthargett
Copy link
Contributor Author

Agreed on semicolons.

re: upgrading to luaparse 0.3, there are some mis-transformations that lua-prettier currently does in Roact and some other codebases I tested. The new luaparse fixes some of those issues, but introduces new ones. I think we may want to look at changing it out altogether, given that we couldn't get clear guidance from the maintainer on how we could help them fix it sooner.

I think what I might like to do is find a good hash of luaparse beyond 0.2 (if one exists), and use that. The second thing I'd like to do is to make the parser configurable so that folks can plug in their own.

how does that strike you?

@suchipi
Copy link
Member

suchipi commented Oct 7, 2020

That sounds good to me

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