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

Why is MagicRegExpMatchArray typed with string | undefined? #233

Open
michaelschufi opened this issue Mar 1, 2023 · 3 comments
Open

Why is MagicRegExpMatchArray typed with string | undefined? #233

michaelschufi opened this issue Mar 1, 2023 · 3 comments
Assignees
Labels
bug Something isn't working

Comments

@michaelschufi
Copy link

michaelschufi commented Mar 1, 2023

Hi

First, thank you so much for this library. I love it so far. I came across a typing issue while trying it out, and thought I'd report it.

🐛 The bug

const regex = createRegExp(
  anyOf('A', 'B', 'C')
    .groupedAs('opponent')
    .and(' ')
    .and(anyOf('X', 'Y', 'Z').groupedAs('self'))
);

// Results in: /(?<opponent>A|B|C) (?<self>X|Y|Z)/

console.log('A Y'.match(regex)?.groups); // { opponent: 'A', self: 'Y' }
console.log('B '.match(regex)?.groups); // undefined
console.log('C Z'.match(regex)?.groups); // { opponent: 'C', self: 'Z' }

The type of each groups is Record<"opponent" | "self", string | undefined>.

Why is the type in
https://github.com/danielroe/magic-regexp/blob/50ac0caa55e6e3fe2c7397297d8a1533190c5012/src/core/types/magic-regexp.ts#L35

defined as string | undefined?

How can the value of the group be undefined while still matching the reg exp?

🛠️ To reproduce

https://stackblitz.com/edit/github-ppjiny?file=index.ts

🌈 Expected behaviour

The type of each groups should be Record<"opponent" | "self", string>.

ℹ️ Additional context

No response

@michaelschufi michaelschufi added the bug Something isn't working label Mar 1, 2023
@michaelschufi michaelschufi changed the title Why is MagicRegExpMatchArray typed with string | undefined? Why is MagicRegExpMatchArray typed with string | undefined? Mar 1, 2023
@didavid61202
Copy link
Collaborator

didavid61202 commented Mar 2, 2023

Hi @michaelschufi,

I think the behavior you're observing is actually working as intended.

We didn't touch any runtime logic (helpers are transform to pure RegExp), so it behave exactly as native RegExp at runtime. The issue here is that /(?<opponent>A|B|C) (?<self>X|Y|Z)/ required both groups opponent and self to be matched, you can actually try with native RegExp:

console.log('B '.match(/(?<opponent>A|B|C) (?<self>X|Y|Z)/)?.groups); 
// return `undefined`

maybe what you want/expect is making the self part optional, you can try:

const regex = createRegExp(
  anyOf('A', 'B', 'C')
    .groupedAs('opponent')
    .and(' ')
    .and(anyOf('X', 'Y', 'Z').groupedAs('self').optionally())
);

this resolve to /(?<opponent>A|B|C) (?<self>X|Y|Z)?/, this will still return undefined if matching 'B ', as the self groups is (or inside) a optional quantifier:

console.log('B '.match(/(?<opponent>A|B|C) (?<self>X|Y|Z)?/)?.groups) 
// return `{ opponent: 'B', self: undefined }`

this is why the type of groups is Record<"opponent" | "self", string | undefined>, as we can still match a certain RegExp but not all of the groups having captured values.

Please let me know if you have any further questions or concerns. Thank you!

@michaelschufi
Copy link
Author

michaelschufi commented Mar 2, 2023

Hi @didavid61202

Thank you for the fast response.

The first part, I completely agree with. Since groups will always be undefined if the regexp doesn't match.

Regarding the second case, you hit the nail on the head. I expect the value part of the type to be string | undefined if and only if I mark at least one group as optional. Otherwise all the groups will always have a value, correct?

Edit: In the meantime I've started to write an enhancement request issue regarding the type resolution for the groups. It would be awesome if the type of each group could be inferred based on the anyOf parameters.

@didavid61202
Copy link
Collaborator

didavid61202 commented Mar 2, 2023

Hi @michaelschufi,

Yes, you are right. If all the groups are not optional, the match result should either be null or matched array with all groups containing value. I think same goes for capturing groups too.

I've benn working on a new type inferrencing for a while, I've put more detail at #235, hopefully will resolve this and many other cases 👍

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