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

feat: add typescript declaration files #11

Conversation

vfreitas-
Copy link
Contributor

@vfreitas- vfreitas- commented Oct 1, 2020

Resolve #5

Checklist

  • Added typescript dependency and a check:types script
  • Added the index.d.ts file as the package.json types root
  • Added a .d.ts definition for each file in the project, with the correct types
  • Added a tsconfig.json file for the check:types script

I think it's all, let me know if I've forgot something :)

@agustito37
Copy link
Contributor

This looks great, thanks for the contribution, I'll get a proper reviewer for this.

@agustito37 agustito37 mentioned this pull request Oct 3, 2020
2 tasks
hg: "height";
}

export default AliasesDictionary;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export default AliasesDictionary;
export default AliasesDictionary;

z: { index: StyleContent; };
};

export default StylesDictionary;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export default StylesDictionary;
export default StylesDictionary;

tsconfig.json Outdated
"esModuleInterop": true
},
"include": ["src/**/*.d.ts"]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
}
}

@agustito37
Copy link
Contributor

agustito37 commented Oct 5, 2020

I have talked with a couple of TS developers and the consensus was that there is a bit of overhead here, having one definition per file. The end-user is going to use just 3 functions mostly: Styles, GlobalStyles, and useStyles; so, maintaining all those files in a pure JS library could be counterproductive. I'm checking if we can reduce that quantity to a small subset.

@vfreitas-
Copy link
Contributor Author

vfreitas- commented Oct 6, 2020

I thought that would be nice to have the definition file for everything because it would be easier to migrate to TS one day. But if it's not the case, maybe we could keep only the definitions that are being exported on the src/index.d.ts file? Or maybe only the ones inside the src/core/stylesManager.d.ts? I can change it, no problem 😄 .

@agustito37
Copy link
Contributor

Agree, I thought about moving to TS, not in the short term I guess given the repo needs more contributions and visibility, having it in pure JS helps a little. For now, we could stick with just the index.d.ts

@vfreitas- vfreitas- force-pushed the feature/issue-5-typescript-declaration-files branch 2 times, most recently from 5680ce0 to 6839518 Compare October 9, 2020 02:42
@vfreitas- vfreitas- force-pushed the feature/issue-5-typescript-declaration-files branch from 6839518 to fc3697e Compare October 9, 2020 02:42
@vfreitas-
Copy link
Contributor Author

Done! I have moved the exported functions to the index.d.ts file and removed the rest. Sorry for the delay. I had some things to do this week 😄

@@ -0,0 +1,19 @@
type StyleObject = {
[key: string]: unknown;
Copy link
Contributor

@agustito37 agustito37 Oct 9, 2020

Choose a reason for hiding this comment

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

Hi! Not sure about the unknown type, seems a little restrictive for this use case, I would rather go with any here. Or maybe don't validate the StyleObject type at all, use object instead (?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, the unknow type is the safe counterpart of any. It's a safer type than any, and a better practice. And the object type refers to all the non-primitive values (like String, null, undefined), but it doesn't allow us to access inner properties, so typing the object map adding a string type to the key is a better approach.

I say this after working with typescript for 3 years +, but it's your call.

TS Playground
Unknown Type

Copy link
Contributor

@agustito37 agustito37 Oct 9, 2020

Choose a reason for hiding this comment

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

Ok, I understand your point, agree, object seems a bad idea; I am not an expert in typescript; what I meant is that I am not sure if forcing the user to cast the value is the way to go, it could be counterproductive.

function doSomething2 (obj: {
    [key: string]: unknown
}) {
    return obj.name.toString(); // Error
}

function doSomething3 (obj: {
    [key: string]: any
}) {
    return obj.name.toString(); // Ok
}

Copy link
Contributor Author

@vfreitas- vfreitas- Oct 9, 2020

Choose a reason for hiding this comment

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

Just to explain it a little more:

This example is exactly where the unknown type provides a safer usage. You can do anything with any, so there is no type safety with it, that's why TS has a strict mode where you block the usage of any (this could be an issue for the users). On the other hand, the unknown needs a cast before usage, so you need to provide the correct type to it before using it, providing a better type safety.

function doSomething2 (obj: {
    [key: string]: unknown
}) {
    return (obj.name as string).toString(); // ok
}

The thing is if something is unknown, how do we can be sure it will have the toString () method? We don't, and if we know it for sure, we simply cast it. In both cases, TS will provide the correct warnings and type safety for us.

But I agree, giving the correct type could be counterproductive for beginners with TS, it's a trade-off. No problem for me if you want to go with any 😄. We can always change it later if there is any kind of issue with one of both uses.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, seems good to me; either way, I don't think this casting thing is going to be something usual. I'll merge this ASAP. Thanks for the explanation and for your contribution!

@agustito37 agustito37 merged commit 2dab777 into rootstrap:master Oct 9, 2020
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.

Typescript types declaration file
2 participants