Skip to content
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

Transition to Typescript #112

Closed
wants to merge 7 commits into from

Conversation

PopGoesTheWza
Copy link

Hi,

I am experimenting with this package and felt like contributing a little. Here's a first draft at switching to Typescript.

Regards,

@@ -14,8 +14,8 @@ jobs:
- 18
- 16
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should support of node 16 be dropped?

@@ -1,12 +1,14 @@
{
"extends": "@sindresorhus/tsconfig",
"compilerOptions": {
"lib": [
"es2021"
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be es2023 if support of node 16 is dropped

"types": "./dist/index.d.ts",
"default": "./dist/index.js"
},
"sideEffects": false,
"engines": {
"node": ">=16"
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should support of node 16 be dropped?

function getPathSegments(path: string) {
const parts = [];
let currentSegment = '';
let currentPart = 'start';
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idea: use Symbols in place of string constants

*/
export function getProperty<ObjectType, PathType extends string, DefaultValue = undefined, ReturnType = ObjectType extends AnObject | AFunction ? (unknown extends Get<ObjectType, PathType> ? DefaultValue : Get<ObjectType, PathType>) : undefined>(
object: ObjectType,
path?: PathType,
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix: original typescript definition had path as manadatory parameter

}
}

return (object === undefined ? defaultValue : object) as ReturnType;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generic type ReturnType may need more work. Currently compatible with original typescript definition but not always accurate.

@sindresorhus
Copy link
Owner

While I appreciate the pull request, it's generally better to open an issue before doing large changes like this. I'm not really interested in moving to TypeScript. It adds a lot of development overhead without much gain. You gain most with TypeScript from codebases that are constantly changing, but this package is pretty much done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants