marijn/orderedmap
1
0
Fork 0
Code Issues 1 Pull requests Releases Activity

chore: fix TypeScript types on constructor and get method #8

Merged
whawker merged 3 commits from master into master 2022-02-21 08:42:15 +01:00
Conversation 5 Commits 3 Files changed 1 +3 -3
whawker commented 2022-02-18 16:02:58 +01:00 (Migrated from github.com)
Copy link

Apologies, I should have picked these type issues up in my pull request yesterday.

  • Allow for the constructor to accept strings (keys) and generic type T values (otherwise it won't accept string keys)
  • Make it explicit that the get method could return undefined.
Apologies, I should have picked these type issues up in my pull request yesterday. * Allow for the constructor to accept strings (keys) and generic type `T` values (otherwise it won't accept string keys) * Make it explicit that the `get` method could return undefined.
marijnh (Migrated from github.com) reviewed 2022-02-20 13:14:38 +01:00
types/index.d.ts
@ -1,7 +1,7 @@
declare class OrderedMap<T = any> {
constructor(content: T[])
private constructor(content: Array<string | T>)
marijnh (Migrated from github.com) commented 2022-02-20 13:14:38 +01:00
Copy link

Why string | T?

Why `string | T`?
whawker (Migrated from github.com) reviewed 2022-02-20 13:44:26 +01:00
types/index.d.ts
@ -1,7 +1,7 @@
declare class OrderedMap<T = any> {
constructor(content: T[])
private constructor(content: Array<string | T>)
whawker (Migrated from github.com) commented 2022-02-20 13:44:26 +01:00
Copy link

@marijnh here's my thinking, apologies if I'm wrong.

If you say your OrderedMap contains numbers OrderedMap<number> you then cannot initialize it with keys new OrderedMap<number>([ 'key1', 1, 'key2', 2]) without type errors.

To get around this, you might try OrderedMap<string | number> but that means ever time you do .get('key1') TypeScript thinks the result is number or string, so you need to do additional work to narrow the type

I added this as convince so you can pass keys in the constructor, without the need for narrowing later on

@marijnh here's my thinking, apologies if I'm wrong. If you say your OrderedMap contains numbers `OrderedMap<number>` you then cannot initialize it with keys `new OrderedMap<number>([ 'key1', 1, 'key2', 2])` without type errors. To get around this, you might try `OrderedMap<string | number>` but that means ever time you do `.get('key1')` TypeScript thinks the result is number or string, so you need to do additional work to narrow the type I added this as convince so you can pass keys in the constructor, without the need for narrowing later on
whawker (Migrated from github.com) reviewed 2022-02-20 13:54:16 +01:00
types/index.d.ts
@ -1,7 +1,7 @@
declare class OrderedMap<T = any> {
constructor(content: T[])
private constructor(content: Array<string | T>)
whawker (Migrated from github.com) commented 2022-02-20 13:54:16 +01:00
Copy link

As an aside, if the constructor took an array of arrays (like Object.entries gives) this would be easier to type.

i.e. new OrderedMap([ ['key1', 1], ['key2, 2] ])

But obviously that would be a breaking change!!

As an aside, if the constructor took an array of arrays (like `Object.entries` gives) this would be easier to type. i.e. `new OrderedMap([ ['key1', 1], ['key2, 2] ])` But obviously that would be a breaking change!!
marijnh (Migrated from github.com) reviewed 2022-02-21 08:06:55 +01:00
types/index.d.ts
@ -1,7 +1,7 @@
declare class OrderedMap<T = any> {
constructor(content: T[])
private constructor(content: Array<string | T>)
marijnh (Migrated from github.com) commented 2022-02-21 08:06:55 +01:00
Copy link

Ah, right, should have read a bit deeper. The thing is that only the documented part of the API is public, so this could just be private constructor()

Ah, right, should have read a bit deeper. The thing is that only the documented part of the API is public, so this could just be `private constructor()`
whawker (Migrated from github.com) reviewed 2022-02-21 08:22:20 +01:00
types/index.d.ts
@ -1,7 +1,7 @@
declare class OrderedMap<T = any> {
constructor(content: T[])
private constructor(content: Array<string | T>)
whawker (Migrated from github.com) commented 2022-02-21 08:22:20 +01:00
Copy link

Yep makes sense. Thanks, will update.

Yep makes sense. Thanks, will update.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
marijn/orderedmap!8
No description provided.