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

instanceof RegExp is not reliable #48

Open
emahuni opened this issue Aug 21, 2022 · 12 comments
Open

instanceof RegExp is not reliable #48

emahuni opened this issue Aug 21, 2022 · 12 comments

Comments

@emahuni
Copy link

emahuni commented Aug 21, 2022

I have an API that runs in nodejs, and regexp matchers fail to run properly. I figured something is making instanceof RegExp to fail. If I do the following from the cli:

node
> anymatch([/conv/], 'conversations') // => true

everything is working as intended, but in the app console:

> anymatch([/conv/], 'conversations') // => false

I managed to isolate the issue to this line:

if (matcher instanceof RegExp) {

it seems like that is producing false even if it is a regexp. So what I did was install another small lib, 'kind-of' and do this on that line and everything works fine again.

// from this ...
if (matcher instanceof RegExp) { //...
// to ...
if (kindOf(matcher) === 'regexp') { //...

This resolves that issue.

I know this issue is not related to the lib per se, but I have tried to isolate code in the API and it seems like there is something deep in some lib in npm that is causing this issue on the RegExp class. It seems time wasting and futile to try and correct it that way, rather than make this lib detect the regexp another way. Will update if I somehow find out what is causing it, coz that in itself is a huge issue that something is messing with the Regexp prototype and causing this issue.
Note that most of the libs used in the backend and frontend are similar, and anymatch runs well in the browser ( we share code in the back and front ends of the whole stack) in which anymatch is used. So this leaves server specific libs faulty, of which there is nothing we are doing to mess with the Regxp prototype.

However, the reason why I am letting you guys know about this is that this lib may suffer with this bug just because of other libs that mess that prototype, meaning it could probably be common in many apps and people just don't know this as the cause. So instead, this lib should just avoid that pitfall and use another way to detect regexps, or just directly do this in this lib:

// shamelessly steal this util function to this lib 😉 beauty of open-source
function isRegexp(val) {
  if (val instanceof RegExp) return true;
  return typeof val.flags === 'string'
    && typeof val.ignoreCase === 'boolean'
    && typeof val.multiline === 'boolean'
    && typeof val.global === 'boolean';
}

// then use it on that offending line
if (isRegexp(matcher)) { //...

From the above code in kind-of, it clearly shows that, yes, they understood that instanceof is not perfect, it will fall through somehow and they then make it brutally get the type by checking known RegExp props. This modification will not impact performance under normal circumstances, just that it will make this lib more RELIABLE, though it will take just a few lines of performance hit when RegExp prototype is not working correctly with instanceof.

@emahuni emahuni changed the title regexp not working at some point in the api instanceof RegExp is not reliable Aug 21, 2022
@paulmillr
Copy link
Contributor

Are you running workers?

@emahuni
Copy link
Author

emahuni commented Aug 22, 2022

No I'm not using workers, why?

@paulmillr
Copy link
Contributor

This most probably happens because RegExp in the worker context is different from a main window's RegExp. You've mentioned "when you're running app console it fails". Which app console is that? Is it just nodejs cli?

@paulmillr
Copy link
Contributor

Basically if this happens, all other instanceof checks are also unreliable in worker context, and many libraries could stop working properly

@emahuni
Copy link
Author

emahuni commented Aug 23, 2022

Oh ok. This is a Node API based on Strapi. But I think what you're saying tallies well. I am creating an REPL in the API where I am doing these tests, this is probably where that RegExp issue is occuring. Let me test by logging within the api without using the console. Will revert with outcome.

Regardless, isn't implementation of a local isRegexp function, like I suggested above, the best way to correct this? Since there really is an issue with RegExp in some contexts.

It'll cater for when RegExp fails, since that's the last resort before it fails.

@emahuni
Copy link
Author

emahuni commented Aug 23, 2022

I just tested Regex independent of anymatch and it works just fine in the console, but not in anymatch. Here is a screenshot:
image
The one at the bottom (in utils._.anymatch) is the one I modified with that function. It just works without any issues. Everything else in the readme examples works for the anymatch import except for regexp. On my modified anymatch, everything just works fine.

I thought that anymatch was the one doing something to RegExp that's why I ran that console.debug command after requiring the package, but no, it's within "anymatch" that this is happening. So the error is happening from anymatch up until you hit the condition where we check if it's a regexp.

I am on Node 14.20, in case this maybe the issue.

I am not too sure why this is the case. Will try to make this reproducible, but the solution is right there, I fixed it and it's working fine with just a few lines of code.

@emahuni
Copy link
Author

emahuni commented Aug 23, 2022

Ok so I think I can reproduce this:

 yarn create strapi-app my-project --quickstart
cd my-project
yarn add anymatch
yarn strapi console

Then test anymatch(/test/, 'testing') there, it won't work, but /test/ instanceof RegExp will work just fine.

@emahuni
Copy link
Author

emahuni commented Aug 23, 2022

That's a pull request for that fix, this is the working version.

@paulmillr
Copy link
Contributor

Again, what's the point if you'll still have all other libraries fail with their instanceof Uint8Array (etc) checks?

@emahuni
Copy link
Author

emahuni commented Aug 26, 2022

The point is to make this work in projects where this would probably not work. I wasn't looking at Uint8Arrays, I focused on RegExp. I never said it didn't work on those. Everything else I tested worked except for regexp only.

This is not specific to that strapi only, it's only that I noticed it there and is a reliable way to reproduce it. If you notice something else, strapi itself is working just fine, instanceof regexp is working as expected, as I demoed above, it's actually something in anymatch package's dependency graph that has issues. There is some conflict somewhere. That environment is only just making it reproducible coz that's where I noticed it.

In other words, this modification makes it work in worker/repl/other weird environments, with a little more work if that's the case, without which it would otherwise fail.

@paulmillr
Copy link
Contributor

paulmillr commented Aug 26, 2022

There is some conflict somewhere

Okay, in this case we need to detect the cause of this conflict.

This fixes anymatch for you -- good. However, I don't want to merge any new code unless it's absolutely our bug. The deep reason why the issue appeared in a first place is required. I maintain many other libraries, which can be affected by it. Maybe the problem is not in the anymatch, but in a shitty way the repl is constructed.

If you are only using anymatch, that's fine. However, if you're using any other libraries, they can also break.

@emahuni
Copy link
Author

emahuni commented Aug 30, 2022

I have confirmed that this has nothing to do with the way the REPL is constructed, do this:

import repl from 'repl';
import anymatch from 'anymatch';
console.log('this is running ok: %o', anymatch(/test/, 'testing'));
// but in the REPL it is not running ok: 
repl.start('> ').context.a = anymatch;

then run the file

in the REPL:

this is running ok: true

a(/test/, 'testing')
// => false
/test/ instanceof RegExp
// => true

same result, this has nothing to do with Strapi. Do it in whatever way you feel makes this simple, but the result is the same. In the REPL, instanceof regexp is correctly deduced, but if you run a debugger and put a breakpoint just on the line where the regexp is being evaluated in anymatch code you will see that anymatch is getting a false.

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

No branches or pull requests

2 participants