chore: assign TS migration #709
Conversation
radko93
requested review from
Esemesek,
grabbou and
thymikee
as code owners
|
Looks like we can remove |
packages/cli/src/tools/assign.ts
Outdated
| return acc; | ||
| }, {}); | ||
| let descriptors = Object.keys(source).reduce( | ||
| (acc: {[k: string]: any}, key) => { |
There was a problem hiding this comment.
how about moving the type to initial value in second argument of reduce? also, please use "unknown" instead of "any"
There was a problem hiding this comment.
Maybe PropertyDescriptorMap it's a better fit than unknown?
(acc: PropertyDescriptorMap, key: string) => {There was a problem hiding this comment.
New update using PropertyDescriptorMap. I had to add a safety check because getOwnPropertyDescriptor also returns undefined.
There was a problem hiding this comment.
Also uknown does not work that great here because you cannot pass it to Object.defineProperties later on.
Removed. |
| } | ||
| return acc; | ||
| }, | ||
| {} as PropertyDescriptorMap, |
There was a problem hiding this comment.
IMO it's a bit also more readable like you said, but we can wait for @thymikee opinion.
There was a problem hiding this comment.
I prefer {} as PropertyDescriptorMap. As long as TS is able to give me correct autocompletion, it's better to rely on inference and type less imho :)
|
You may want to add |
@assuncaocharles missed that from other PRs, thanks |
Summary:
Migrated
assign.jsandisValidPackageName.jsto typescript. Related to #683Is
isValidPackageNameused by external tool? Because it's not being used inside CLI so maybe we can remove it.