Fix typing performance regression #2166
Conversation
There was a problem hiding this comment.
Thanks for the time and effort to find a fix for issue #2138 @felixmeziere!
I run the test locally and no errors were found.
@adrai when you have the change could you please approve workflow run so we can double check on GitHub actions?.
Reviewing the code I found a possible improvement on FilterKeysByContext.
@felixmeziere Could you please try if the change works also in your project?
typescript/t.d.ts
Outdated
| type FilterKeysByContext<Keys, Context extends any> = Context extends string | ||
| ? Keys extends `${infer Prefix}${_ContextSeparator}${Context}${infer Suffix}` | ||
| ? `${Prefix}${Suffix}` | ||
| : never | ||
| : Keys; |
There was a problem hiding this comment.
FilterKeysByContext is only used by ParseKeys which already check that Context value is a string before so it can be changed to something like this:
type FilterKeysByContext<Keys, Context extends string> =
Keys extends `${infer Prefix}${_ContextSeparator}${Context}${infer Suffix}`
? `${Prefix}${Suffix}`
: never;With this change we can also remove extends any which doesn't add any type specifity
There was a problem hiding this comment.
@marcalexiei thanks, actually the most important part is the const ... so I improved the fix further so that the code is more readable (amended the commit sorry).
There was a problem hiding this comment.
I applied code formatting which changed other places in the file, not sure if it's legit or not, let me know :)
There was a problem hiding this comment.
actually the most important part is the const ... so I improved the fix further so that the code is more readable
👌
I applied code formatting which changed other places in the file, not sure if it's legit or not, let me know :)
Strange, code base already follows formatting rules.
Actually these changes do not follow prettier styles so related PR check is failing.
Try running:
npm run format:fixThere was a problem hiding this comment.
done thanks, for some reason my IDE wouldn't pick up the proper prettier config...
felixmeziere
force-pushed
the
master
branch
3 times, most recently
from
555c9b7
to
c708c3a
Compare
felixmeziere
changed the title
|
Thank you... it's included in v23.11.0 |
| @@ -128,12 +128,13 @@ export type ParseKeys< | |||
| KPrefix = undefined, | |||
| Keys extends $Dictionary = KeysByTOptions<TOpt>, | |||
| ActualNS extends Namespace = NsByTOptions<Ns, TOpt>, | |||
| const Context extends TOpt['context'] = TOpt['context'], | |||
There was a problem hiding this comment.
The added const here throws an error TS1277: 'const' modifier can only appear on a type parameter of a function, method or class in a project of mine. I suspect this shouldn't be the case?
There was a problem hiding this comment.
I'm also encountering into the same problem.
I'm using typescript v5.4.4.
There was a problem hiding this comment.
This issue will be tracked via #2168
#2138 (comment)