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

Glob **/!(*-dbg).@(js) is wrongly translated into RegExp and thus DOES match -dbg.js files #93

Closed
kristian opened this issue Aug 26, 2021 · 9 comments
Labels
bug Something isn't working

Comments

@kristian
Copy link

The Glob:

**/!(*-dbg).@(js)

Which should match any .js files that do not end with -dbg is wrongly translated to:

^(?:(?:^|[\\/]|(?:(?:(?!(?:^|[\\/])\.{1,2}(?:[\\/]|$)).)*?)[\\/])(?:(?!(?:[^\\/]*?-dbg).@(js))[^\\/]*?)\.(js))$

Please note how for the negative backreference, the glob pattern .@(js) is literally taking into the regex and is not translated into a regexp / excapted itself? Expected RegExp would have been:

^(?:(?:^|[\\/]|(?:(?:(?!(?:^|[\\/])\.{1,2}(?:[\\/]|$)).)*?)[\\/])(?:(?!(?:[^\\/]*?-dbg)\.(js))[^\\/]*?)\.(js))$
@mrmlnc mrmlnc added the bug Something isn't working label Oct 31, 2021
@mrmlnc
Copy link
Contributor

mrmlnc commented Oct 31, 2021

The bug related to #84.

Similar patterns that lead to false behavior:

!(*.d).{ts,tsx} → match 'file.d.ts'

@mrmlnc
Copy link
Contributor

mrmlnc commented Jan 2, 2022

I did a little research on this problem. The problem is in this block. Here we get everything after the closing parenthesis and put it in the pattern. This is an error because any other nested expression can come after the closing parenthesis.

Unfortunately, I'm not sure that this problem can be fixed without rewriting most of this module. Literally, we need to stop and parse the following expression after the parenthesis and then go to back and insert the result.

@jonschlinkert
Copy link
Member

jonschlinkert commented Jan 2, 2022

@mrmlnc nice work on debugging and figuring out where the problem is!

Unfortunately, I'm not sure that this problem can be fixed without rewriting most of this module.

It should be possible without having to refactor to use an AST. FWIW, I create multiple versions of this with an AST first, but all approaches were slower due to object allocation etc. But I'm open to switching back to the AST, or maybe just pushing a version with an AST up to a branch.

Before that, I have an idea of how to do this without much code. @mrmlnc what are your thoughts on what the resulting regex should be to make this work?

edit: or if you want to give it a try, I can just add a comment with my thoughts on how it could be solved. But I'd be happy if you or someone else wants to solve this however you think it should be solved.

@mrmlnc
Copy link
Contributor

mrmlnc commented Jan 2, 2022

Heh, I can make this work by calling the parse function on the rest of string.

if (token.inner.includes('*') && (rest = remaining()) && /^\.[^\\/.]+$/.test(rest)) {
+  // Disabling the `fastpaths` option due to a problem with parsing strings as `.ts` in the pattern like `**/!(*.d).ts`.
+  const value = parse(rest, { ...options, fastpaths: false }).tokens
+    .map(token => token.output || token.value)
+    .join('');

-  output = token.close = `)${rest})${extglobStar})`;
+  output = token.close = `)${value})${extglobStar})`;
}

It passes all the tests, including the ones I added.

Previous regexp:

// !(*.d).{ts,tsx}
/^(?:(?=.)(?:(?!(?:[^/]*?\.d).{ts,tsx})[^/]*?)\.(ts|tsx))$/
                              ^----------- wrong output

The current regexp:

// !(*.d).{ts,tsx}
/^(?:(?=.)(?:(?!(?:[^/]*?\.d)\.(ts|tsx))[^/]*?)\.(ts|tsx))$/
                               ^----------- correct output

But it doesn't work for the original pattern from this issue.

@jonschlinkert, if the suggested solution (calling the parse function) suits you, then I can finish it and submit PR.

upd. Even easier. This version works even with the original pattern from this issue.

if (token.inner.includes('*') && (rest = remaining()) && /^\.[^\\/.]+$/.test(rest)) {
+  // Disabling the `fastpaths` option due to a problem with parsing strings as `.ts` in the pattern like `**/!(*.d).ts`.
+  const value = parse(rest, { ...options, fastpaths: false }).output;

-  output = token.close = `)${rest})${extglobStar})`;
+  output = token.close = `)${value})${extglobStar})`;
}

Previous regexp:

// **/!(*-dbg).@(js)
/^(?:(?:^|\/|(?:(?:(?!(?:^|\/)\.).)*?)\/)(?:(?!(?:[^/]*?-dbg).@(js))[^/]*?)\.(js))$/
                                                              ^------------- wrong output

The current regexp:

// **/!(*-dbg).@(js)
/^(?:(?:^|\/|(?:(?:(?!(?:^|\/)\.).)*?)\/)(?:(?!(?:[^/]*?-dbg)\.(js))[^/]*?)\.(js))$/
                                                              ^------------- correct output

@jonschlinkert
Copy link
Member

jonschlinkert commented Jan 2, 2022

if the suggested solution (calling the parse function) suits you, then I can finish it and submit PR.

Yes that's great! was just doing something much more complicated, your solution is better. Thanks and nice work!

(this is why I love open source. both the collaboration, but also I get to see how other devs solve problems in ways I didn't think of!)

@mrmlnc
Copy link
Contributor

mrmlnc commented Jan 2, 2022

Fixed by #102 and will be released soon under version 2.3.1.

@mrmlnc mrmlnc closed this as completed Jan 2, 2022
@kristian
Copy link
Author

kristian commented Jan 2, 2022

Thanks @mrmlnc and @jonschlinkert! One workaround less in our code! Highly appreciated! 👍

@jonschlinkert
Copy link
Member

@kristian thank you for creating the issue and for the great description to help us get this figured out!

@Jock0707
Copy link

Thanks for the help

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

4 participants