-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Add type definitions for recoil #44756
Conversation
@csantos42 Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 - keep an eye on this comment as I'll be updating it with information as things progress. Code ReviewsThis PR adds a new definition, so it needs to be reviewed by a DT maintainer before it can be merged. Status
Once every item on this list is checked, I'll ask you for permission to merge and publish the changes. Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 44756,
"author": "csantos42",
"owners": [],
"dangerLevel": "ScopedAndConfiguration",
"headCommitAbbrOid": "f6ddc8d",
"headCommitOid": "f6ddc8da25b2f47bf62aa132a87fd2ce24d82eb4",
"mergeIsRequested": false,
"stalenessInDays": 0,
"lastCommitDate": "2020-05-29T18:05:09.000Z",
"lastCommentDate": "2020-05-29T17:40:30.000Z",
"reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/44756/files",
"hasMergeConflict": false,
"authorIsOwner": false,
"isFirstContribution": true,
"popularityLevel": "Well-liked by everyone",
"anyPackageIsNew": true,
"packages": [
"recoil"
],
"files": [
{
"filePath": "types/recoil/index.d.ts",
"kind": "definition",
"package": "recoil"
},
{
"filePath": "types/recoil/recoil-tests.ts",
"kind": "test",
"package": "recoil"
},
{
"filePath": "types/recoil/tsconfig.json",
"kind": "package-meta",
"package": "recoil"
},
{
"filePath": "types/recoil/tslint.json",
"kind": "package-meta",
"package": "recoil"
}
],
"hasDismissedReview": false,
"ciResult": "pass",
"lastReviewDate": "2020-05-29T18:19:28.000Z",
"reviewersWithStaleReviews": [
{
"reviewedAbbrOid": "22328be",
"reviewer": "smmoosavi",
"date": "2020-05-21T00:52:24Z"
},
{
"reviewedAbbrOid": "53fcac4",
"reviewer": "acutmore",
"date": "2020-05-16T16:48:28Z"
}
],
"approvalFlags": 4,
"isChangesRequested": false
} |
Inspecting the JavaScript source for this package found some properties that are not in the .d.ts files. recoil (unpkg)was missing the following properties:
|
👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings. Let’s review the numbers, shall we? These typings are for a package that doesn’t yet exist on master, so I don’t have anything to compare against yet! In the future, I’ll be able to compare PRs to recoil with its source on master. Comparison details 📊
|
I'm not too familiar with TypeScript but I don't see any indicator of covariance for the read-only values. Is that part of this? |
@davidmccabe Do you mean to mark the argument of the setValue as Sadly, the keyword |
can export the type like |
I'm not sure the RecoilValue type is right. If RecoilValue can be used in I think I read the wrong code..it does look like it is set correctly. |
Referencing this PR and stealing over portions that make sense DefinitelyTyped/DefinitelyTyped#44756
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.
RecoilState should be both covariance and contravariance. because it is both readable and writable.
but right now any RecoilState
can be assigned to any RecoilState
without any error.
@smmoosavi, @acutmore Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review? |
I've done some work independently, not realizing this PR existed. I have a couple of the tricky cases worked out. The types are pretty well tested and are quite robust. Feel free to incorporate anything from that branch to this PR. |
@amcasey, @smmoosavi, @acutmore Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review? |
@csantos42 Thanks! I think that addresses @sheetalkamat's concern. She knows more about modules resolution than I do and may be able to suggest a way to get back your tidy multi-file structure, but it seems like there's enough excitement around these types to handle that in a separate PR. Did you have a chance to look at @kolodny and pull out possible improvements? @smmoosavi @Gregoor @acutmore @renanmav Remaining concerns? |
@amcasey yes, we should be good to go on our end :) |
@amcasey the issue with problemconst stringAtom: RecoilState<string> = {} as any;
const stringOrNumberAtom: RecoilState<string|number> = stringAtom // no error. it should detect error, it should check contravariance. because RecoilState is writable
const [value, setValue] = useRecoilState<string|number>(stringOrNumberAtom)
setValue(5) // no error, ok, SetterOrUpdater<string|number> should accept number
const value2 = useRecoilValue(stringAtom) // expect value2 should be string, but it is 5 solutionwe can add another key to writable states to check contravariance. type NodeKey = string
declare type CB<T> = (t: T) => void;
export class AbstractRecoilValue<T> {
' covariance-value': [T];
' contravariance-value': [CB<T>];
key: NodeKey;
constructor(newKey: NodeKey);
}
export class AbstractRecoilValueReadonly<T> {
' covariance-value': [T];
key: NodeKey;
constructor(newKey: NodeKey);
} also |
thanks @smmoosavi ! Clever-- I didn't think of using a function type to ensure contravariance, I'll make that change now :) |
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 @smmoosavi's concerns have been addressed and there's tremendous interest in this PR, so I'm going to preemptively merge it. @csantos42, please be responsive to subsequent feedback if it turns out I've merged too soon.
Thanks a lot @amcasey! And thanks to everyone that reviewed/contributed/commented on this PR! I'll be sure to monitor any subsequent feedback promptly |
Recoil 0.0.8 is published on npm, but the types are still missing a few things, like |
hi @kingdaro, I'm working on this now, should be done some time tonight. Thanks! |
* Initial types * TypeScript types for Recoil * fix lint errors * fix tests * Remove unused types * fix prettier error * Improve types in response to feedback * Remove loadable accessors * Fix broken tests * Fix reference to React types * Fix lint error * Consolidate types into one file to match src structure * fix contravariance of writeable state:
* Initial types * TypeScript types for Recoil * fix lint errors * fix tests * Remove unused types * fix prettier error * Improve types in response to feedback * Remove loadable accessors * Fix broken tests * Fix reference to React types * Fix lint error * Consolidate types into one file to match src structure * fix contravariance of writeable state:
Please fill in this template.
npm test
.)npm run lint package-name
(ortsc
if notslint.json
is present).Select one of these and delete the others:
If adding a new definition:
.d.ts
files generated via--declaration
dts-gen --dt
, not by basing it on an existing project.tslint.json
should be present and it shouldn't have any additional or disabling of rules. Just content as{ "extends": "dtslint/dt.json" }
. If for reason the some rule need to be disabled, disable it for that line using// tslint:disable-next-line [ruleName]
and not for whole package so that the need for disabling can be reviewed.tsconfig.json
should havenoImplicitAny
,noImplicitThis
,strictNullChecks
, andstrictFunctionTypes
set totrue
.