-
-
Notifications
You must be signed in to change notification settings - Fork 83
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
Better JSDoc types #231
Better JSDoc types #231
Conversation
src/Tokenizer.js
Outdated
* @return {any} | ||
* @typedef {any} T | ||
* @param {T} value | ||
* @return {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.
I'm trying to show that what you give it is what you get back, as 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.
this could use @template 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.
after a thought - while this works OK with @template
it doesn't quite showcase how this function should be used and it is specific to this library and its context. It should always be used together with alloc
and I feel that both alloc
and dealloc
should share the same return type. In this case I would keep using the @template
but the accepted and returned types should be T[]
instead of plain T
, because we always allocate and deallocate arrays.
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 think that's a limitation of the way the code is written - If this package had any documentation it'd be good for it to go in there. I agree that it's confusing, but I think a lot of this codebase is. A lot of functions have side effects on shared global data, or are aliases to other functions, or change data types randomly in order to have "terse" code. It feels like there's too much to fix in terms of type safety.
I'm making another comment in this thread in a sec, basically saying I'd be happy for you to take over since I imagine this PR will stall.
Pull Request Test Coverage Report for Build 27c44ce386aa07c6c2785babfb86216bf855799f-PR-231
💛 - Coveralls |
If these look good I'll compile a single index.d.ts of all types and upload it to npm as |
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 is good work overall, but personally I would just convert the whole thing to TS if we care about providing good typings by default. Using JSDoc to do this is quirky and far lesser-known skill nowadays, so this doesn't necessarily improve the situation for potential contributors.
@thysultan do you prefer TS or JSDoc?
src/Tokenizer.js
Outdated
@@ -101,15 +103,16 @@ export function token (type) { | |||
|
|||
/** | |||
* @param {string} value | |||
* @return {any[]} | |||
* @return {[]} |
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.
won't this be an equivalent to never[]
or something? from what I remember this allocates Element[]
, I think this wouldn't compile correctly when declared like this when using TS, not sure what about JSDoc though
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.
Hmm. It returns exactly []
. Not sure what it's going to be used for afterwards but it's []
at this point
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.
From types perspective, it is important how it is going to be used later - the declared type must match how it is going to be used, just take a look at this demo:
https://www.typescriptlang.org/play?#code/GYVwdgxgLglg9mABAQwDarhAFASgFyIDaAuogN4BQi1iATgKZQi1IkUC+FFUAngA71EAJRCpBAXnK8BeAOS1R9WQBpEANzQh6BAM5RaMMAHNOAE3oRUyBoggI9dAiLFc0GbDgB0fEDoAWWLQ4QA
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.
Ok fair. I'll use unknown[]
. Good call
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.
In a similar fashion unknown[]
is not a good fit here because it would require either casting or type refining down the road - we want the result of alloc
to be consumable by other code. That's why generic (T[]
) would be preferred here.
Look what happens with unknown
:
https://www.typescriptlang.org/play?#code/GYVwdgxgLglg9mABAQwDarhAFASgFyLgDWYcA7mANoC6iA3gFCLOIBOAplCK0jQwL4MGUAJ4AHdogBKIVJIC89URLwByVrPaqANIgBuaEOwIBnKKxhgA5oIAm7CKmQdEEBGbYEZcoQdYp0THZbREU0DGwcBnCg2wA6MRATAAssViiGNzAPVgAmL01QgIjgygAGaiEgA
src/Tokenizer.js
Outdated
* @return {any} | ||
* @typedef {any} T | ||
* @param {T} value | ||
* @return {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.
this could use @template T
@Andarist Thanks for all the feedback! That's very helpful. I went and pushed your suggested changes about As for changing the codebase to Typescript, I don't think it's worth the fuss for @thysultan who doesn't seem to already use TS in any way, and didn't seem familiar with it in #208. It would add unnecessary complexity to an otherwise simple and small project that can be adequately typed by JSDoc. Furthermore, the author likes to use very terse code in a handful of places that Typescript just wouldn't allow - you'd have to either rewrite it (like I was trying to do with |
Converting a project to TS is only ever for the author anyway. Users of a library will always need an index.js+index.d.ts. I think it's more important to get that d.ts out the door so people can actually use this lib with their TS code. I'll upload to @types/stylis as soon as we can confirm the emitted declaration from JSDoc is sufficient |
Will be merging this: DefinitelyTyped/DefinitelyTyped@master...heyheyhello:master |
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.
Playing around with the readme examples in Typescript at https://github.com/heyheyhello/DefinitelyTyped/blob/cd84939cbf43f7112bce12b6a3cd6a09c20a0197/types/stylis/stylis-tests.ts
src/Middleware.js
Outdated
|
||
* @typedef {( | ||
* element: Element, | ||
* index?: number, | ||
* children?: Element[], | ||
* children?: (Element | 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.
See readme.md where apparently this can be a string
src/Middleware.js
Outdated
* children?: Element[] | string, | ||
* root: Element, | ||
* type: string, | ||
* props: string[] | 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.
Similarly, this is apparently a string in some cases. Though all references in the codebase treat it as an array (using methods like .join()
)
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.
Can we put the typedef's(single line) where they are created in the code i.e for the Element typedef we can call it node as it is returned from the node
function and put it next to the node
functions return JSDOC. For example in https://github.com/thysultan/stylis.js/blob/eff8dc8a0ca05c3a80054ca57915e8a5dba0ab9e/src/Tokenizer.js#L21 to have the following:
/**
* @typedef {{parent?: node, children?: node[] | string, root: node, type: string, props: string[] | string, value: string, length: number, return: string, line?: number, column?: number}} node
* @param {string} value
* @param {nod} root
* @param {node?} parent
* @param {string} type
* @param {string[]} props
* @param {node[]} children
* @param {number} length
* @return node
*/
Same thing for middleware in https://github.com/thysultan/stylis.js/blob/eff8dc8a0ca05c3a80054ca57915e8a5dba0ab9e/src/Middleware.js#L41 to have the following:
/**
* @typedef {(element: node, index?: number, children?: (node | string)[], callback?: middleware) => string | void} middleware
* @param {middleware[]} collection
* @return {middleware}
*/
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.
We want Node
(or Element
, whatever we are going to have it called) to be reusable across different declarations, not sure how defining a @typedef
within the same block as a function signature would work. Even if it would work it kinda blends together for me and I wouldn't call this more readable in the end of a day. Defining different types of nodes (CommentNode
, DeclarationNode
, etc) is also worth exploring - so I think keeping them separate (but probably in the same file) would improve readability.
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.
Defining different types of nodes (CommentNode, DeclarationNode, etc)
That might be too much to represent in JSDocs given the constraints of jsdocs and my reluctants to spear head a full-on TypeScript re-write. If it works i think an inline @typedef has the benefit of highlighting where to the type is sourced/created from avoiding indirection; in contrast: in its free-floating form it raises the question of whether it's from global source.
PR is now open at DefinitelyTyped/DefinitelyTyped#46171 @Andarist would you like to be reviewer and/or co-owner as per bot comment
|
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 would like to wait a couple of days for @thysultan opinion about this before moving this work to DefinitelyTyped. From my POV it would be better to have those types defined here (even if using JSDoc) to avoid desynchronization of the codebase and types.
I believe we should try to typecheck the codebase to assess that what has been typed is in fact correct - and to do that I think we should add jsconfig.json
to the project (or tsconfig.json)
. At least it's the only thing that made VScode to start typechecking this for me. I've checked out your branch and fixed a bunch of minor issues - there are some other type problems though and I will have to look into them a little bit later.
src/Tokenizer.js
Outdated
@@ -101,15 +103,16 @@ export function token (type) { | |||
|
|||
/** | |||
* @param {string} value | |||
* @return {any[]} | |||
* @return {[]} |
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.
From types perspective, it is important how it is going to be used later - the declared type must match how it is going to be used, just take a look at this demo:
https://www.typescriptlang.org/play?#code/GYVwdgxgLglg9mABAQwDarhAFASgFyIDaAuogN4BQi1iATgKZQi1IkUC+FFUAngA71EAJRCpBAXnK8BeAOS1R9WQBpEANzQh6BAM5RaMMAHNOAE3oRUyBoggI9dAiLFc0GbDgB0fEDoAWWLQ4QA
src/Utility.js
Outdated
*/ | ||
export function append (value, array) { | ||
return array.push(value), value | ||
} | ||
|
||
/** | ||
* @param {string[]} array | ||
* @param {function} callback | ||
* @param {ArrayMapCallback} callback |
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.
While this is, of course, OK - I don't particularly feel that having this referenced like that is a significant gain. Especially given that in this particular case this type isn't 100% correct - a true ArrayMapCallback
should be generic. It's also the only place this type is being used now so I would prefer just writing this thing here inline
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.
It's mostly to differentiate from Middleware which looks super similar and middleware is sometimes passed around as the variable callback
🙄
I agree that the type is not as generic as it could be, but it's actually only used twice in the whole codebase and my preference would be to not have combine
at all - use [].map().join()
. The author likes to have a lot of aliases though, and I don't think it's worth trying to convince them to change that
src/Tokenizer.js
Outdated
* @return {any} | ||
* @typedef {any} T | ||
* @param {T} value | ||
* @return {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.
after a thought - while this works OK with @template
it doesn't quite showcase how this function should be used and it is specific to this library and its context. It should always be used together with alloc
and I feel that both alloc
and dealloc
should share the same return type. In this case I would keep using the @template
but the accepted and returned types should be T[]
instead of plain T
, because we always allocate and deallocate arrays.
I thought this was only the case for the consumer? |
I agree that it's normally ideal for types to exist in the source codebase, but I think its unrealistic in this case. #208 has been open for months. There's not much show of interest from @thysultan about moving forward and I don't blame them - having d.ts in the codebase would be a huge overhaul. The types I have in DT aren't reproducible from
This will do type checking. But you'll notice that the codebase is not written with type safety in mind - even the nature of having helper functions like
I just don't think the author wants type problems to be fixed or the project to move more towards TypeScript (or be ported to TS). @thysultan can you comment on your interest or how important it is to you to re-write in that direction? #208 is months old and I know there are people who wanted types available for this as soon as 4.0 was out. I'll push to DefinitelyTyped, and if this ever gets merged or type support becomes first class in this codebase I'll be happy to remove the types from DT as documented in https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/README.md#removing-a-package |
JSDocs yes, not so much a TypeScript re-write. See my comments on my suggestions on this PR. |
The ultimate goal of this work is to both document the codebase better and to generate
Yeah, I realize the codebase is using some unsafe, from the perspective of the types, patterns but sprinkling few
I think it would be better to close that DefinitelyTyped PR for like a week or two. Let us figure this out here in that timeframe and if we won't be able to get the job done you will be able to reopen that PR there. |
😍 Nice! It's a lot of code but looks good to me. I imagine it's a bit overwhelming to anyone not familiar with types but certainly a better way to go. |
* @return {Middleware} | ||
*/ | ||
export function rulesheet (callback) { | ||
return function (element) { | ||
if (!element.root) | ||
// @ts-ignore |
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.
Not a fan of the ts-ignore comments, considering this commit essentially create a full type-definition we could just shop a typings.d.ts
definition file with this in it instead of JSDoc comments?
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.
Not a fan of the ts-ignore comments
Plan to remove them, they were only somewhat helpful to me when I've been typing this.
considering this commit essentially create a full type-definition we could just shop a typings.d.ts definition file with this in it instead of JSDoc comments?
Yeah, we can totally do that - my idea was just that the types were sort-of already here, so I've tweaked those instead of providing a separate set of types. Would you like me to bring back old&simple JSDocs and gather all of those types within .d.ts
file?
src/Serializer.js
Outdated
@@ -1,11 +1,11 @@ | |||
import {IMPORT, COMMENT, RULESET, DECLARATION} from './Enum.js' | |||
import {strlen, sizeof} from './Utility.js' | |||
|
|||
/** @typedef {import('./Middleware.js').Element} Element */ |
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 typedef's not global? i.e is there a need to import them?
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've pushed out some changes. They are by no means final - just a base for further discussion, but actually it shouldn't take that much to finish this work as this is already functioning quite well. We only need to decide how granular etc our types should be.
Apart from a couple of places where I've added @ts-ignore
comments because variables were reused for new data of a different type the codebase was pretty straightforward to type. While having type-checking on in the codebase brings value and could potentially catch some issues early here, those suppressing comments litter the code a little bit and it's hard to put them at a faulty expression level so they work now on the whole statements which tend to be long. So I would say that in this case removing them and turning off the type-checking for internals would be preferred.
@@ -1,20 +1,74 @@ | |||
/** |
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.
"had" to add those @type
declarations because those use var
and not const
so their type is widened to a string
but actually we want some of them to act as sentinel values in unions, so it is required for them to have a "discrete" value
* array: string[], | ||
* ) => string | ||
* } ArrayMapCallback | ||
* @typedef {NodeBase & { |
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.
sharing this common part like that makes IDE hints somewhat unreadable as it tries to expand those types instead of using their names, I think it's some quirk with JSDoc support as this doesn't happen as easily when working with regular TS code
for this reason, it might make sense to just inline this "base" everywhere, those typedefs would get lengthy, but we can put the common part last to counteract this a little bit, worth a discussion
}} RulesetNode | ||
|
||
* @typedef {NodeBase & { | ||
type: '@keyframes' |
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.
Creating separate types for all possible nodes is not technically correct because the runtime doesn't actually care for the most part - so it is possible to feed such a string to compile
that one of produced Nodes would not match against any of those that are defined here.
Specifying them here makes working with the compiled output easier though because it is natural to differentiate node types based on their .type
property and this works perfectly with TS - after such a check it is able to limit the type of a node within a conditional block to the one matched by a condition, like in here TS can know that for a RuleNode props
has to be an array of strings
* @typedef {NodeBase & { | ||
type: '@counter-style' | ||
props: string[] | ||
children: DeclarationNode[] |
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 is, somewhat, an interesting case as well - I've limited the children
here to DeclarationNode[]
as it's the only thing that makes sense but it's also not guaranteed by the runtime. But again - by grouping different types and providing logical children
/parent
/root
for all of them we would provide a better DX. The "correct" AST allows only for certain relations between different node types and knowing them is very valuable for developers - when working with Babel plugins I use @babel/types
docs a lot.
/** | ||
* @param {number} |
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.
old definitions were not picked up by the IDE at all - I think those can only be used for actual function declarations and not for defining types when assigning to a variable like this
return {value: value, root: root, parent: parent, type: type, props: props, children: children, line: line, column: column, length: length, return: ''} | ||
} | ||
|
||
/** | ||
* @param {string} value | ||
* @param {Element} root | ||
* @param {Node} source |
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've renamed this param as root
was IMHO a confusing name for this - given that we have a .root
property on Nodes
* @param {Element} root | ||
* @param {Element?} parent | ||
* @param {Node?} root | ||
* @param {Node?} parent | ||
* @param {number} index | ||
* @param {number} offset | ||
* @param {string[]} rules | ||
* @param {number[]} points | ||
* @param {string} type |
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.
potentially we could explore limiting this to a particular union of strings and define the whole function in a similar fashion as node
* Types for Stylis 4.0 * Add default export for Stylis Co-authored-by: Piotr Błażejewicz (Peter Blazejewicz) <peterblazejewicz@users.noreply.github.com> * Improved T generics thysultan/stylis#231 (comment) Co-authored-by: Piotr Błażejewicz (Peter Blazejewicz) <peterblazejewicz@users.noreply.github.com>
@Andarist I think your changes would be great as a PR to update the typings on DefinitelyTyped. The typings that are available+published now as @types/stylis work well for developers using stylis, but your changes would benefit people trying to write middleware and hack on stylis itself. Specifically adding node subtypes and renaming parameters (i.e root to source) would benefit developers who are working with stylis internals - which may be groups like emotion and styled-components (who are currently stuck on V3). If you're interested in opening a PR there I'll approve them and the tests can be edited to cover more type-specific usecases like writing middleware. |
Sorry for being MIA lately - had a lot on my shoulders and couldn't find time to get back to this PR. @heyheyhello I think we have every intention of shipping those types (after some redacting) as part of the Stylis itself. We just need to figure out how we want to do this - which shouldn't take us that long. I hoped you wouldn't rush with this DefinitelyTyped PR too much as now we'll be in this weird situation where types will be published (soon-ish) both within |
@Andarist There's no issue with that. It's something that DefinitelyTyped plans for and has deprecation warnings for. It's easy to open a PR in DT to have the types taken down :) If you shipped types tomorrow no one installing @types/stylis would see an issue; the typings would still resolve. I didn't feel rushed with DT, but I've seen many PRs stay open for months or years so why not have the ball rolling now? I didn't see any reason to stop progress |
What will happen with people who have already installed |
Also a huge push for me is that my memory's not great - it's been 20 days since this PR was opened and honestly I don't remember writing the types. There's a small window before I move onto other things... |
Thought I'd mention now that I've been using @types/stylis for a bit... The huge amount of commonly named exports that stylis has is problematic: things like Not sure the best way forward. If not remove exports then how about renaming or prefixing? |
@heyheyhello we can't just rename them in v4 as that would be a breaking change, so even if we would have provided aliases for those the problem for you would still persist. It really feels like your IDE's algorithm could be improved to prefer local variables? Can't it be configured somehow? I also don't see this much of a problem - one library's API choices can't be dictated by IDEs behaviors. |
@heyheyhello Why would it try to autocomplete for exports you haven't imported? unless you're importing everything? import * as stylis from 'stylis'
stylis.next(...) |
That's a great idea thanks. Star import is the way to go. Also my editor does auto imports. I think vscode offers it by default? So importing only one thing from stylis tells the editor to be ready to offer all symbols and just patch the import statement as needed. Sorry I thought it was a common feature. Good fix! |
@heyheyhello great work on Shame about the poorly typed JSDoc annotations... but now with |
* Types for Stylis 4.0 * Add default export for Stylis Co-authored-by: Piotr Błażejewicz (Peter Blazejewicz) <peterblazejewicz@users.noreply.github.com> * Improved T generics thysultan/stylis#231 (comment) Co-authored-by: Piotr Błażejewicz (Peter Blazejewicz) <peterblazejewicz@users.noreply.github.com>
I work almost exclusively in Typescript and not having type support is a show stopper for what is otherwise a lovely project. I looked at #208 and while I think it has good intentions the types are vague. That's not the PR author's fault -
tsc
just extracts the types from JSDoc. The JSDoc typings could be better.I went through and tried to fix it up. I removed all the instances of
object
andfunction
with their real types. There are a few errors and maybe you can correct me on some assumptions I had to make about what is a possible data type but I think these are good types - specifically the use ofMiddleware
as a type to explain that a function is middleware (or returns middleware in the case ofrulesheet()
).I arbitrarily picked Middleware.js to write the JSDoc
@typedef
but they could be anywhere - maybe index.js would be a better place?I'll leave comments in the PR code to explain what I'm trying to do