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

simpleComputed regex edge case #358

Open
UnrefinedBrain opened this issue Sep 29, 2024 · 8 comments
Open

simpleComputed regex edge case #358

UnrefinedBrain opened this issue Sep 29, 2024 · 8 comments
Labels
bug Something isn't working

Comments

@UnrefinedBrain
Copy link

UnrefinedBrain commented Sep 29, 2024

const regex = /const\s+([a-zA-Z0-9_$]+)\s*=\s*computed\(\s*\(\)\s*=>\s*{([^{}]*(?:{[^{}]*}[^{}]*)*)}\s*\)/gs

Regex is not able to reliably match the entire code block because it lacks the capability to match unknown numbers of balanced pairs. If there is more than one level of nesting, the current regex here will not match it

Example of issue (regex101 link):

const foo = computed(() => {
  if (bar) {
    if (baz) {
    }
  }
});

highly suggest switching to an AST-based approach rather than regex for this. It might look like this:

const ast = babel.parse(sourceCode); // any ESTree-compatible parser would work
const nodes = esquery.query(ast, 'CallExpression[callee.name="computed"][arguments.0.type="ArrowFunctionExpression"] .arguments:nth-child(1) .body');
for (const node of nodes) {
  const newlines = sourceCode.slice(node.range[0], node.range[1]).match(/\n/g)?.length ?? 0;
  if (newLines > maxNewLineCount) {
    const lineNumber = node.loc.start.line;

    reportProblem(lineNumber, 'too many newlines in computed');
  }
}
@rrd108
Copy link
Owner

rrd108 commented Sep 29, 2024

Thanks for the suggestion.

As I try to avoid using a parser, I should check if we really need it here.

@David-Pena
Copy link
Collaborator

@rrd108 I played around with this issue (as it comes from Reddit) and when there are multiple conditions inside the computed property it indeed gets weird and the regex start to become more complex.

I didnt try it but something that might work is changing the logic to not use regex either but just find open and close brackets and do the magic (something we have in some rules already like the functionSize and ... probably other one I can't remember rn)

@rrd108
Copy link
Owner

rrd108 commented Sep 29, 2024

Yes. That was the one.

@UnrefinedBrain
Copy link
Author

UnrefinedBrain commented Sep 29, 2024

As I try to avoid using a parser

I'm curious why this is? Regex is the wrong tool for the job here and a parser/AST is the right tool. I'm not trying to be rude and ofc you can do what you want with your project, I'm just a bit confused why creating maintainability issues, reliability issues, and edge case tradeoffs when you could avoid all of it

@rrd108
Copy link
Owner

rrd108 commented Sep 30, 2024

I'm curious why this is? Regex is the wrong tool for the job here and a parser/AST is the right tool. I'm not trying to be

We just missing some benchmarking to check it. If you can do some for us you are very welcome to do so!

@rrd108
Copy link
Owner

rrd108 commented Sep 30, 2024

Esprima seems to be a bteer option then babel.

@UnrefinedBrain
Copy link
Author

Esprima seems to be a bteer option then babel.

Esprima doesn't support typescript syntax as far as I know. Babel does and it can generate an ESTree-compliant AST that can be used with various libraries like ast-types and esquery

not like SWC which has its own non-ESTree AST and isn't compatible with the myriad of libraries for working with ESTree ASTs

Another option is to use ast-grep API which has its own vue language support

@rrd108
Copy link
Owner

rrd108 commented Sep 30, 2024

Esprima doesn't support typescript syntax as far as I know. Babel does and it can generate an ESTree-compliant AST that can be used with various libraries like ast-types and esquery

Yes, I saw that. I created a dedicated issue for this. see #368

@David-Pena David-Pena added the bug Something isn't working label Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants