feat: add getReplacementDescription util#488
feat: add getReplacementDescription util#488gameroman wants to merge 1 commit intoes-tooling:mainfrom
getReplacementDescription util#488Conversation
|
|
||
| export function getReplacementDescription( | ||
| replacement: ModuleReplacement | ||
| ): string | undefined { |
There was a problem hiding this comment.
| ): string | undefined { | |
| switch (replacement.type) { | |
| case 'documented': | |
| return undefined; | |
| default: | |
| return replacement.description; | |
| } |
we should narrow it rather than checking the shape manually i think
There was a problem hiding this comment.
I'm thinking this is better because if we ever add description to documented type we won't need to change this function and don't accidentally forget to
There was a problem hiding this comment.
i think we should have to update the code.
its better to narrow the union correctly instead of narrowing the shape manually.
this description stuff seems to be eating up more time than it should be. its specifically a tagged union so we are made to narrow the type. the correct way to narrow the type is this switch. all of this came from you wanting to save some lines of code (wanting all replacements to have a description so you don't have to do any narrowing). but that would've meant pretending documented replacements have a description just to save some bytes, which isn't right. this way, if we ever do introduce a description, it isn't a breaking change.
There was a problem hiding this comment.
Added the util to npmx pr instead 👍
|
in the npmx PR i was specific that this belongs there rather than here because i knew the suggestion would be to put it here 😅 i'm not strongly opposed to it being here but no other consumer needs it right now. e.g. the CLI doesn't need to do this as we just narrow and handle each type. this right now is pretty much an npmx-only function. @dreyfus92 what do you think? |
|
exporting some utils would be cool, but in this case maybe the narrow is too simple to warrant a fn? |
🔗 Linked issue
📚 Description
Add
getReplacementDescriptionutilSee #481 and npmx-dev/npmx.dev#2068 (comment)