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

"*" not valid for any if used as second match, and result confusion #12

Open
Portur opened this issue Mar 6, 2019 · 5 comments
Open

Comments

@Portur
Copy link

Portur commented Mar 6, 2019

Im sure im doing this right.... but my tests result in the following

const domainMatch = require('wildcard');
domainMatch ( '4' , [ '*' ] ).length                        //-> 0    //1 expected
domainMatch ( '4000' , [ '*' , 'somethingelse' ] ).length   //-> 0    //1 expected
domainMatch ( 'localhost' , ['*'] ).length                  //-> 0    //1 expected
domainMatch ( 'localhost' , [ 'localhost' ] ).length        //-> 1    //1 expected
domainMatch ( '*' , [ '*' ] ).length                        //-> 1    //1 expected

so, according to the doc

domainMatch ( 'a.b.*', [ 'a.b.c', 'a.b', 'a', 'a.b.d' ]).length   //->3  //3 expected
//but
domainMatch ( '*.*.*', [ 'a.b.c', 'a.b', 'a', 'a.b.d' ]).length  //->4  //4 expected
domainMatch ( 'a.b.c, a.b, a, a.b.d', '*.*.*')                  //false //true expected

Summary
which is then where the confusion is. the first item is matched against the second, however the second being '*' will not match.

domainMatch ('localhost', [ '*' ]).length //->0  //1 expected
domainMatch ('*', [ 'localhost' ]).length //->1  //1 expected

also

domainMatch( '*, localhost ', 'localhost' )       //-> false      //true expected
domainMatch( '*', 'localhost' )                   //-> Array[1]   //true expected

I understand the issue is more matching the first to the rest. Building this example also helped me clear it up but I just wanted to let you know my thoughts.

The first item needs to be a string array and the second a string, however the second should be of type Array and the second string, but the second can't be *. If I got that right

@Portur Portur changed the title "*" not valid for any "*" not valid for any if used as second match, and result confusion Mar 6, 2019
@DamonOehlman
Copy link
Owner

@Portur thanks for logging the issue and the detailed investigation that you took on. I'm going to do my best to take a look at the issue properly in the next few days.

@AnandChowdhary
Copy link

@DamonOehlman, did you find time to have a second look at this? 😄

@DamonOehlman
Copy link
Owner

@AnandChowdhary @Portur to be honest, I got completely distracted by other things (as you can probably tell). I've added myself a calendar note to take a look at this tonight.

@DamonOehlman
Copy link
Owner

Hey folks - apologies again for the delay. I investigated the above (and added a couple of additional tests to cover the * case). I think what you are experiencing comes down to a couple of things:

  1. I never bumped and published a version after merging Properly support wildcard when not in a section of its own #11 which makes the "naked wildcard" (*) behave as it should. So tests against wildcard@1.x would fail in that case, which is what you are seeing above.

  2. The use case that you described where there is a comma separated list of patterns, e.g. domainMatch( '*, localhost ', 'localhost' ) is not something that is supported by wildcard and thus it will always return false. So yes you are right the format the wildcard expects arguments is:

wildcard(matchPattern, test)

Where test can either be a string, or an array of strings that will be filtered. As an aside, I think adding the support for checking on an array was a mistake and something I should have built differently.


Anyway, I have bumped the version of wildcard to 2.0.0 as technically the way the * is interpreted now vs before is definitely a breaking change and deserves a major upgrade. If you feel like taking it for a spin to see if it behaves how you think it should then please do and let me know.

@AnandChowdhary
Copy link

This is great, thank you!

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

3 participants