Rewrite in TypeScript #96

rafaelramalho19 · 2019-11-19T00:36:27Z

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:

#86: Improve the TypeScript types

IssueHunt has been backed by the following sponsors. Become a sponsor

.eslintrc

.gitignore

tsconfig.json

source/index.ts

package.json

test/index.ts

source/index.ts

sindresorhus · 2019-11-21T12:13:42Z

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 old index.d.ts as close as possible.
Don't use any or object. Use a specific type whenever possible, and if it's supposed to be non-specific, use unknown.
https://github.com/sindresorhus/typescript-definition-style-guide has a lot of useful info about best practices.
Revert: ed72882#diff-21e9ddd93162651bd36f6e5bbfca8460L1-R14

rafaelramalho19

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 old index.d.ts as close as possible.

Don't use any or object. Use a specific type whenever possible, and if it's supposed to be non-specific, use unknown.

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.

source/index.ts

rafaelramalho19 · 2019-11-25T10:46:28Z

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 old index.d.ts as close as possible.

Don't use any or object. Use a specific type whenever possible, and if it's supposed to be non-specific, use unknown.

https://github.com/sindresorhus/typescript-definition-style-guide has a lot of useful info about best practices.

Revert: ed72882#diff-21e9ddd93162651bd36f6e5bbfca8460L1-R14

Regarding:
1️⃣ I also moved the exports to the bottom of the file so it doesn't get in the way of reading the main part of the source code. Is that okay with you?
2️⃣ Done.
3️⃣ I've removed all I could. The ones that remained seem a bit necessary 🤔
4️⃣ Thanks for the read 👍
5️⃣ Done, I had to do that with the import * from x syntax since it was throwing deprecated warnings.

package.json

source/index.ts

sindresorhus · 2019-11-25T12:55:28Z

source/index.ts

+
+	@param keys - The keys of the items to reset.
+	*/
+	reset(...keys: string[]): void {


Same as above.

What typing do you suggest here? Because string is already equal to K (which is just the key of Conf)

The answer is in the existing TS definition:

conf/index.d.ts

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.

I've not been able to make it work, it always says this:

Do you have any suggestions for my problem? :/

source/index.ts

test/index.ts

sindresorhus · 2019-11-25T12:57:48Z

Try to make sure that the output dist/index.d.ts matches the old index.d.ts as close as possible.

I don't think you have done this thoroughly enough. The types in the existing type definition are better in many places.

rafaelramalho19 · 2019-11-29T12:46:17Z

Try to make sure that the output dist/index.d.ts matches the old index.d.ts as close as possible.

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.

sindresorhus · 2020-01-01T11:43:12Z

There are still some unresolved inline comments like #96 (comment)

package.json

sindresorhus · 2020-01-01T11:31:24Z

source/index.ts

+	@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 {


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 {

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 returnundefined`.

Is also raises the question:

Do we want undefined, but declared, values in the store to be returned or fallbacked?

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.

sindresorhus · 2020-01-01T11:34:24Z

source/index.ts

+
+	@param keys - The keys of the items to reset.
+	*/
+	reset(...keys: string[]): void {


The answer is in the existing TS definition:

conf/index.d.ts

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.

sindresorhus · 2020-01-01T11:35:14Z

source/index.ts

+}
+
+export type Migrations<T> = {
+	[key: string]: (store: Conf<T>) => void;


Suggested change

[key: string]: (store: Conf<T>) => void;

[semverSpecifier: string]: (store: Conf<T>) => void;

sindresorhus · 2020-01-01T11:36:35Z

source/index.ts

+	@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 {


This should be strongly-typed.

conf/index.d.ts

Lines 316 to 319 in 97bcc91

onDidChange<K extends keyof T>(

key: K,

callback: (newValue?: T[K], oldValue?: T[K]) => void

): () => void;

sindresorhus · 2020-01-01T11:36:53Z

source/index.ts

+	/**
+	Delete all items.
+	*/
+	clear(): void{


Suggested change

clear(): void{

clear(): void {

sindresorhus · 2020-01-01T11:39:57Z

source/index.ts

+				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);


Don't use any.

sindresorhus · 2020-01-01T11:41:15Z

source/index.ts

+	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;


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.

sindresorhus · 2020-01-01T11:44:18Z

source/index.ts

+	_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)


Suggested change

const newerVersions: string[] = Object.keys(migrations)

const newerVersions = Object.keys(migrations)

sindresorhus · 2020-02-16T15:15:00Z

@rafaelramalho19 Still interested in finishing this?

rafaelramalho19 · 2020-02-19T12:11:33Z

@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.

sindresorhus · 2020-02-22T08:54:06Z

Sure, no worries at all. I totally understand. TypeScript is hard. I think so too.

sindresorhus · 2020-02-22T08:54:55Z

Moved to #86.

refactor: change source code to typescript

84c740e

satazor reviewed Nov 19, 2019

View reviewed changes

.eslintrc Outdated Show resolved Hide resolved

satazor reviewed Nov 19, 2019

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

satazor reviewed Nov 19, 2019

View reviewed changes

tsconfig.json Outdated Show resolved Hide resolved

rafaelramalho19 added 2 commits November 19, 2019 11:55

fix: EOL

32e0f45

fix: allow dynamic import in tests to work

daebf20

papb reviewed Nov 19, 2019

View reviewed changes

source/index.ts Show resolved Hide resolved

fix: typescript not uses proper imports

ed72882

rafaelramalho19 requested a review from sindresorhus November 21, 2019 10:39

sindresorhus changed the title ~~refactor: change source code to typescript~~ Rewrite in TypeScript Nov 21, 2019

sindresorhus requested changes Nov 21, 2019

View reviewed changes

rafaelramalho19 commented Nov 21, 2019

View reviewed changes

source/index.ts Show resolved Hide resolved

source/index.ts Show resolved Hide resolved

rafaelramalho19 added 2 commits November 22, 2019 18:43

fix: change typescript typings

9345d57

fix: update import syntax

76f70ab

sindresorhus reviewed Nov 23, 2019

View reviewed changes

source/index.ts Outdated Show resolved Hide resolved

fix: add docs and refactor types

0b0a98d

rafaelramalho19 added 2 commits November 25, 2019 10:48

chore: fix reserved keys typing

42c900e

fix: test typings

d655983

rafaelramalho19 requested a review from sindresorhus November 25, 2019 10:51

fix: optional params

846e9e9

sindresorhus requested changes Nov 25, 2019

View reviewed changes

fix: update test typing

b09a636

rafaelramalho19 added 2 commits December 4, 2019 14:43

fix: improve typescript typings and documentation

e32335f

fix: test typings

c08e4d4

rafaelramalho19 requested a review from sindresorhus December 4, 2019 15:09

papb mentioned this pull request Dec 4, 2019

fix: unable to import module from typescript jonschlinkert/time-stamp#18

Open

sindresorhus requested changes Jan 1, 2020

View reviewed changes

sindresorhus mentioned this pull request Feb 22, 2020

Improve the TypeScript types #86

Closed

6 tasks

sindresorhus closed this Feb 22, 2020

sindresorhus mentioned this pull request Feb 24, 2020

Move to TypeScript #104

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite in TypeScript #96

Rewrite in TypeScript #96

rafaelramalho19 commented Nov 19, 2019 •

edited by issuehunt-oss bot

sindresorhus commented Nov 21, 2019 •

edited

rafaelramalho19 left a comment

rafaelramalho19 commented Nov 25, 2019

sindresorhus Nov 25, 2019

rafaelramalho19 Dec 4, 2019

sindresorhus Jan 1, 2020

rafaelramalho19 Feb 19, 2020

sindresorhus commented Nov 25, 2019

rafaelramalho19 commented Nov 29, 2019

sindresorhus commented Jan 1, 2020 •

edited

sindresorhus Jan 1, 2020

rafaelramalho19 Jan 2, 2020

sindresorhus Jan 2, 2020

sindresorhus Jan 1, 2020

sindresorhus Jan 1, 2020

sindresorhus Jan 1, 2020

sindresorhus Jan 1, 2020

sindresorhus Jan 1, 2020

sindresorhus Jan 1, 2020

sindresorhus Jan 1, 2020

sindresorhus commented Feb 16, 2020

rafaelramalho19 commented Feb 19, 2020

sindresorhus commented Feb 22, 2020

sindresorhus commented Feb 22, 2020

	[key: string]: (store: Conf<T>) => void;
	[semverSpecifier: string]: (store: Conf<T>) => void;

	onDidChange<K extends keyof T>(
	key: K,
	callback: (newValue?: T[K], oldValue?: T[K]) => void
	): () => void;

	const newerVersions: string[] = Object.keys(migrations)
	const newerVersions = Object.keys(migrations)

Rewrite in TypeScript #96

Rewrite in TypeScript #96

Conversation

rafaelramalho19 commented Nov 19, 2019 • edited by issuehunt-oss bot

Referenced issues

sindresorhus commented Nov 21, 2019 • edited

rafaelramalho19 left a comment

Choose a reason for hiding this comment

rafaelramalho19 commented Nov 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Nov 25, 2019

rafaelramalho19 commented Nov 29, 2019

sindresorhus commented Jan 1, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Feb 16, 2020

rafaelramalho19 commented Feb 19, 2020

sindresorhus commented Feb 22, 2020

sindresorhus commented Feb 22, 2020

rafaelramalho19 commented Nov 19, 2019 •

edited by issuehunt-oss bot

sindresorhus commented Nov 21, 2019 •

edited

sindresorhus commented Jan 1, 2020 •

edited