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
Rewrite in TypeScript #96
Rewrite in TypeScript #96
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The doc comments in index.d.ts should be moved to the new index.ts.
- Try to make sure that the output
dist/index.d.ts
matches the oldindex.d.ts
as close as possible.- Don't use
any
orobject
. Use a specific type whenever possible, and if it's supposed to be non-specific, useunknown
.- https://github.com/sindresorhus/typescript-definition-style-guide has a lot of useful info about best practices.
- Revert: ed72882#diff-21e9ddd93162651bd36f6e5bbfca8460L1-R14
Thank you for the review Sindre. I will read that today and will try to fix as much as I can asap.
Regarding: |
|
||
@param keys - The keys of the items to reset. | ||
*/ | ||
reset(...keys: string[]): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What typing do you suggest here? Because string
is already equal to K
(which is just the key of Conf)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The answer is in the existing TS definition:
Line 289 in 97bcc91
reset<K extends keyof T>(...keys: K[]): void; |
Yes, both resolve to a string, but having it strongly typed will let editors auto-complete the key names and let TS enforce only valid key names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you have done this thoroughly enough. The types in the existing type definition are better in many places. |
You're right Sindre. Sorry about that. I've been a bit busy lately but don't worry about this becoming stale, I'll just pick it up as soon as I can. |
There are still some unresolved inline comments like #96 (comment) |
@param key - The key of the item to get. | ||
@param defaultValue - The default value if the item does not exist. | ||
*/ | ||
get(key: string, defaultValue?: T): T | undefined { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have an overload that makes the return value T
when defaultValue
is given.
So the definitions should be:
get(key: string): T | undefined {
get(key: string, defaultValue: T): T {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure about the return type of the second overload?
There's a wide range of inputs to the function where it could return undefined.
one example, assuming the store has:
[cool-key]: undefined
using get('cool-key', 'this should be the fallback
)will return
undefined`.
Is also raises the question:
- Do we want undefined, but declared, values in the store to be returned or fallbacked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The store cannot have undefined
:
The value must be JSON serializable. Trying to set the type undefined, function, or symbol will result in a TypeError.
My suggestion assumes you also do #96 (comment) to all the set/get/reset/etc methods, so they're strongly typed.
|
||
@param keys - The keys of the items to reset. | ||
*/ | ||
reset(...keys: string[]): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The answer is in the existing TS definition:
Line 289 in 97bcc91
reset<K extends keyof T>(...keys: K[]): void; |
Yes, both resolve to a string, but having it strongly typed will let editors auto-complete the key names and let TS enforce only valid key names.
} | ||
|
||
export type Migrations<T> = { | ||
[key: string]: (store: Conf<T>) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[key: string]: (store: Conf<T>) => void; | |
[semverSpecifier: string]: (store: Conf<T>) => void; |
@param key - The key wo watch. | ||
@param callback - A callback function that is called on any changes. When a `key` is first set `oldValue` will be `undefined`, and when a key is deleted `newValue` will be `undefined`. | ||
*/ | ||
onDidChange(key: string, callback: (...args: unknown[]) => void): () => unknown { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be strongly-typed.
Lines 316 to 319 in 97bcc91
onDidChange<K extends keyof T>( | |
key: K, | |
callback: (newValue?: T[K], oldValue?: T[K]) => void | |
): () => void; |
/** | ||
Delete all items. | ||
*/ | ||
clear(): void{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clear(): void{ | |
clear(): void { |
const initializationVector = data.slice(0, 16); | ||
const password = crypto.pbkdf2Sync(this.encryptionKey, initializationVector.toString(), 10000, 32, 'sha512'); | ||
const decipher = crypto.createDecipheriv(encryptionAlgorithm, password, initializationVector); | ||
const slicedData: any = data.slice(17); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use any
.
get store(): {[key: string]: T} { | ||
try { | ||
const data: string | Buffer = fs.readFileSync(this.path, this.encryptionKey ? null : 'utf8'); | ||
const dataString = this._encryptData(data) as string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should not use as string
here as encryptData could return a Buffer. Generally, stay away from as
unless necessary as it could easily hide type errors.
_migrate(migrations: Migrations<T>, versionToMigrate: string): void { | ||
let previousMigratedVersion: string = this._get(MIGRATION_KEY, '0.0.0') as string; | ||
|
||
const newerVersions: string[] = Object.keys(migrations) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const newerVersions: string[] = Object.keys(migrations) | |
const newerVersions = Object.keys(migrations) |
@rafaelramalho19 Still interested in finishing this? |
Maybe someone else can pick up on this? I'm more than happy to let someone else finish this, I guess I'm not experienced enough in Typescript for converting all these code. Sorry for the long wait Sindre. |
Sure, no worries at all. I totally understand. TypeScript is hard. I think so too. |
Moved to #86. |
Closes #86
It's my first time working on Typescript, so this can be a bit bad.
IssueHunt Summary
Referenced issues
This pull request has been submitted to:
IssueHunt has been backed by the following sponsors. Become a sponsor