[parse] - Project cleanup and fix for Object.set/save #52592
Conversation
Result of the following command: `npm run prettier -- --write types/parse/**/*.ts`
- Remove unnecessary ts4.0 folder - Remove `no-self-import` rule
|
@ridem Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — I'm the local bot who will help you through the process of getting things through. This is a live comment which I will keep updated. 1 package in this PRCode ReviewsBecause you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it. Status
All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes. Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 52592,
"author": "ridem",
"headCommitOid": "df7a2894ebb4a1fd1c8ad5e62cfe1215c4533c0d",
"lastPushDate": "2021-05-01T09:13:13.000Z",
"lastActivityDate": "2021-05-01T11:20:09.000Z",
"maintainerBlessed": false,
"mergeOfferDate": "2021-05-01T10:52:08.000Z",
"mergeRequestDate": "2021-05-01T11:20:09.000Z",
"mergeRequestUser": "ridem",
"hasMergeConflict": false,
"isFirstContribution": true,
"tooManyFiles": false,
"popularityLevel": "Well-liked by everyone",
"pkgInfo": [
{
"name": "parse",
"kind": "edit",
"files": [
{
"path": "types/parse/index.d.ts",
"kind": "definition"
},
{
"path": "types/parse/node.d.ts",
"kind": "definition"
},
{
"path": "types/parse/package.json",
"kind": "package-meta-ok"
},
{
"path": "types/parse/parse-tests.ts",
"kind": "test"
},
{
"path": "types/parse/react-native.d.ts",
"kind": "definition"
},
{
"path": "types/parse/ts3.6/index.d.ts",
"kind": "definition"
},
{
"path": "types/parse/ts3.6/node.d.ts",
"kind": "definition"
},
{
"path": "types/parse/ts3.6/parse-tests.ts",
"kind": "test"
},
{
"path": "types/parse/ts3.6/react-native.d.ts",
"kind": "definition"
},
{
"path": "types/parse/ts4.0/index.d.ts",
"kind": "definition"
},
{
"path": "types/parse/ts4.0/node.d.ts",
"kind": "definition"
},
{
"path": "types/parse/ts4.0/parse-tests.ts",
"kind": "test"
},
{
"path": "types/parse/ts4.0/react-native.d.ts",
"kind": "definition"
},
{
"path": "types/parse/ts4.0/tsconfig.json",
"kind": "package-meta-ok"
},
{
"path": "types/parse/ts4.0/tslint.json",
"kind": "package-meta-ok"
},
{
"path": "types/parse/tslint.json",
"kind": "package-meta-ok"
},
{
"path": "types/parse/v1/index.d.ts",
"kind": "definition"
},
{
"path": "types/parse/v1/parse-tests.ts",
"kind": "test"
}
],
"owners": [
"ullisenmedia",
"dpoetzsch",
"jaeggerr",
"flavionegrao",
"wesleygrimes",
"owsas",
"agoldis",
"AlexandreHetu",
"dplewis",
"yomybaby",
"pocketcolin",
"rdhelms",
"jlnquere",
"tybi",
"RaschidJFR",
"jeffgukang",
"buitanloc",
"LinusU",
"JeromeDeLeon",
"kentrh",
"L3K0V",
"swittk"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Well-liked by everyone"
}
],
"reviews": [
{
"type": "approved",
"reviewer": "rdhelms",
"date": "2021-05-01T10:48:59.000Z",
"isMaintainer": false
}
],
"ciResult": "pass"
} |
|
🔔 @ullisenmedia @dpoetzsch @jaeggerr @flavionegrao @wesleygrimes @owsas @agoldis @AlexandreHetu @dplewis @yomybaby @pocketcolin @rdhelms @jlnquere @tybi @RaschidJFR @JeffGuKang @buitanloc @LinusU @JeromeDeLeon @kentrh @L3K0V @swittk — please review this PR in the next few days. Be sure to explicitly select |
| @@ -457,25 +457,9 @@ declare global { | |||
| remove: this["add"]; | |||
| removeAll: this["addAll"]; | |||
| revert(...keys: Array<Extract<keyof (T & CommonAttributes), string>>): void; | |||
| save<K extends Extract<keyof T, string>>( | |||
| attrs?: | |||
| | (((x: T) => void) extends (x: Attributes) => void | |||
There was a problem hiding this comment.
This is true:
((x: T) => void) extends (x: Attributes) => void- when
Tis defined and just has optional keys - when
Tis alreadyParse.Attributes
There was a problem hiding this comment.
@ridem This would break the following case (which should be added as a test case - it doesn't look we currently have one)
async function testSave(
objUntyped: Parse.Object,
objTyped: Parse.Object<{ example: boolean; someString: string }>,
) {
// $ExpectError
await objTyped.save({ example: undefined })
}
That should error with Type 'undefined' is not assignable to type 'boolean'. but your change would allow it.
There was a problem hiding this comment.
To clarify here: the only reason why this case should error (in my opinion) is because the attributes had been typed without allowing undefined. For the case where the attributes ARE typed either as Partial or with properties typed as undefined, I'm fine allowing those to be set as undefined.
My problem was just with the Partial<T>, but I think Pick<T, K> | T seems to work great.
|
👋 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?
First off, note that the system varied slightly between these two runs, so you’ll have to take these measurements with a grain of salt. Looks like there were a couple significant differences—take a look at worst-case duration for getting quick info at a position to make sure everything looks ok. |
typescript-bot
added
the
Perf: Worse
There was a problem hiding this comment.
@ridem What's your use case that led you to make this PR? The change as you have it would break existing functionality.
Also, it looks like you had some autosave lint config applied here, which changed all the ' -> ".
Interesting...I didn't know DefinitelyTyped updated their single/double quote preference a couple months ago
| @@ -457,25 +457,9 @@ declare global { | |||
| remove: this["add"]; | |||
| removeAll: this["addAll"]; | |||
| revert(...keys: Array<Extract<keyof (T & CommonAttributes), string>>): void; | |||
| save<K extends Extract<keyof T, string>>( | |||
| attrs?: | |||
| | (((x: T) => void) extends (x: Attributes) => void | |||
There was a problem hiding this comment.
@ridem This would break the following case (which should be added as a test case - it doesn't look we currently have one)
async function testSave(
objUntyped: Parse.Object,
objTyped: Parse.Object<{ example: boolean; someString: string }>,
) {
// $ExpectError
await objTyped.save({ example: undefined })
}
That should error with Type 'undefined' is not assignable to type 'boolean'. but your change would allow it.
typescript-bot
added
the
Revision needed
typescript-bot
moved this from Waiting for Code Reviews
to Needs Author Action
in New Pull Request Status Board
|
@ridem One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you! |
typescript-bot
removed
the
Revision needed
typescript-bot
moved this from Needs Author Action
to Waiting for Code Reviews
in New Pull Request Status Board
|
Thanks for the quick answer, @rdhelms.
Several reasons:
Not having autocompletion suggestions on things like:
Basically being able to do things like this: type BookAttributes = {
author: string; title: string;
}
class Book extends Parse.Object<BookAttributes> {
constructor(options?: any) {
super("Book", options);
}
setTolkienBook(attrs: Partial<BookAttributes>) {
this.set({
...attrs,
author: "Tolkien"
})
}
}
This is a warmup for which I read the About Thanks a lot for clarifying what the intent was with this. I've added a commit that includes:
For my use case n°2, I understand that this isn't possible if you do rely on forbidding AFAIK Parse isn't meant to throw when you pass an
There are other places which would need guards to forbid class Test extends Parse.Object<{title?: string;}> {}
new Test("Test", { title: undefined })or function testSet( objUntyped: Parse.Object) {
// $ExpectType false | Object<Attributes>
objUntyped.set({ propA: undefined });
}Is it the types job to forbid these microsoft/TypeScript#43805 is an interesting read about how the issue might be tackled in future releases. To sum up what the PR is doing:
|
|
Updated numbers for you here from 90ef822. Nice job, these numbers look better. Comparison details 📊
First off, note that the system varied slightly between these two runs, so you’ll have to take these measurements with a grain of salt. It looks like nothing changed too much. I won’t post performance data again unless it gets worse. |
typescript-bot
added
Perf: Same
|
@ridem Thanks for the really useful explanation and summary. I'm testing your changes locally a bit before approving, but that all seems very promising. We hadn't noticed any issues with Intellisense in our codebase (EDIT: I was able to reproduce some Intellisense issues with the currently published version - we just happened to be using architectural patterns that didn't hit those cases), because rather than using classes that extend Parse Object classes, we just create and use the Parse Objects directly, and But it seems like you've improved the handling for cases like extended classes, so that seems great 👍🏻 For using class Book extends Parse.Object<BookAttributes> {
constructor (options: BookAttributes) {
super('Book', options)
}
setTolkienBook () {
this.set({
author: undefined, // Errors, as expected
})
}
}
class Book extends Parse.Object<BookAttributes> {
constructor (options: BookAttributes) {
super('Book', options)
}
setTolkienBook () {
this.set({}) // ~~Should not work~~ EDIT: My bad - this actually should be fine
}
}I will also say that we've had a relatively limited scope of use cases and documented examples of how people are using this package, so I think it would be interesting to compare notes some more to make sure that we're taking as many cases, patterns, and architectures into account as possible. @RaschidJFR or others, would you care to share how using this package (particularly the generically-typed |
types/parse/parse-tests.ts
Outdated
| /** | ||
| * Parse doesn't throw when a value is `undefined`, | ||
| * but saves won't be taken into account. | ||
| */ | ||
|
|
||
| // $ExpectError | ||
| await objTyped.save({ example: undefined }); | ||
|
|
||
| // $ExpectError | ||
| await objTyped.save("example", undefined); | ||
|
|
||
| // $ExpectError | ||
| await objTypedOptional.save({ example: undefined }); | ||
|
|
||
| // $ExpectError | ||
| await objTyped.save('wrongProp', true); | ||
| await objTypedOptional.save("example", undefined); |
There was a problem hiding this comment.
Ah...I misinterpreted what you were saying in your other comment...so actually in these cases, since the attributes type explicitly types the properties as undefined, we would have allowed setting these. I do see how it could be beneficial to prevent ever setting a value to be undefined in order to force/guide users more towards .unset...but I do think that may be a bit heavy-handed.
More and more, I think the core issue for your case anyway is some difference between the behavior of class Thing extends Parse.Object<SomeAttributes> and new Parse.Object<SomeAttributes>('Thing'). It seems like the extended class is losing some TS context somehow...
There was a problem hiding this comment.
I would vote that we support these test cases:
// $ExpectError
await objTyped.save({ example: undefined });
// $ExpectError
await objTyped.save("example", undefined);
// $ExpectType Object<OptionalObjectAttributes>
await objTypedOptional.save({ example: undefined });
// $ExpectType Object<OptionalObjectAttributes>
await objTypedOptional.save("example", undefined);Which, by removing the Required<> from your save() and set() types, I believe should all pass and also still support Intellisense for all kinds of attribute types, partial or not.
typescript-bot
moved this from Waiting for Code Reviews
to Needs Maintainer Action
in New Pull Request Status Board
typescript-bot
moved this from Needs Maintainer Action
to Waiting for Code Reviews
in New Pull Request Status Board
|
@rdhelms 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? |
There was a problem hiding this comment.
@ridem Ok - I think maybe I've come up with a solution to help with your Intellisense and extended class use case, while maintaining the existing behavior of the package thus far:
save<K extends Extract<keyof T, string>>(
attrs?: Pick<T, K> | T | null,
options?: Object.SaveOptions,
): Promise<this>;
save<K extends Extract<keyof T, string>>(key: K, value: T[K], options?: Object.SaveOptions): Promise<this>;
set<K extends Extract<keyof T, string>>(
attrs: Pick<T, K> | T | null,
options?: Object.SetOptions,
): this | false;
set<K extends Extract<keyof T, string>>(key: K, value: T[K], options?: Object.SetOptions): this | false;I think this is possible now partly due to updates by the TS compiler since the time when these types were first written. The | T part of the union type is what particularly helps Intellisense, and that idea came from react's types for setState here:
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/react/index.d.ts#L482
If that seems good to you, it would be great if you could adjust that but keep all the other improvements with linting, etc. And I would love to get a future PR figured out for handling dot notation with template literal types 😄
typescript-bot
added
the
Revision needed
typescript-bot
moved this from Waiting for Code Reviews
to Needs Author Action
in New Pull Request Status Board
|
@ridem One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you! |
There was a problem hiding this comment.
Thanks @ridem - I think this is great. It's some much needed cleanup, improved transparency, and a great fix for Intellisense on save and set.
I really appreciate the compromise we came to, because I know for my company's particular use we highly value how TS will error if we try to set something as undefined - we see that as a bug and never want to allow it in our own codebase. But since not everyone would want to be that strict, I think having the ability to opt out of those errors by using Partial or | undefined on an attribute is really valuable. And for those wanting even looser typings, we still have the parameter-less flavor of Parse.Object() which lets people set and save whatever attributes they want.
It's incredibly ironic that we independently found and cited the exact same line of react's types...initially when I saw your change with SafeAttributes, I was only focused on the Required part and didn't even think about the | T part. And then after my own research attempts, I just ended up at the exact thing that you had already found 😅
Let us know how the support for dot notation goes - we would love to get support for that, and are happy to help out if the effort becomes problematic.
typescript-bot
moved this from Needs Author Action
to Waiting for Code Reviews
in New Pull Request Status Board
|
Also, it seems like the CI builds are failing for something unrelated to parse - not sure what that's about. Looks like maybe it's from |
typescript-bot
moved this from Waiting for Code Reviews
to Needs Author Action
in New Pull Request Status Board
|
@sandersn It looks like our package's build is failing with errors related to rxjs...is it possible that we need to make updates related to your recent commit? |
|
Yep, you'll need to merge from master to get rid of those errors. |
|
Updated numbers for you here from 3f1a400. Comparison details 📊
First off, note that the system varied slightly between these two runs, so you’ll have to take these measurements with a grain of salt. Wow, it looks like all the big movers moved in the right direction! Way to go! 🌟 I won’t post performance data again unless it gets worse. |
typescript-bot
added
Perf: Better
|
@rdhelms |
typescript-bot
moved this from Needs Author Action
to Waiting for Code Reviews
in New Pull Request Status Board
|
@rdhelms 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? |
|
Updated numbers for you here from 82f87d2. Comparison details 📊
First off, note that the system varied slightly between these two runs, so you’ll have to take these measurements with a grain of salt. It looks like nothing changed too much. I won’t post performance data again unless it gets worse. |
typescript-bot
added
Perf: Same
typescript-bot
added
Owner Approved
typescript-bot
moved this from Waiting for Code Reviews
to Waiting for Author to Merge
in New Pull Request Status Board
|
Ready to merge |
typescript-bot
moved this from Waiting for Author to Merge
to Recently Merged
in New Pull Request Status Board
|
I just published |
|
I just published |
|
@ridem @rdhelms @sandersn Thank you very much. But i have some problem when using ES 6 Minimized version as mentioned in official repo: import Parse from 'parse/dist/parse.min.js';When using the method above, the autocomplete will not showing. How to show the autocomplete even i use ES 6 Minimized version? Updated (Solved) as mentioned in these comment, you should specify the definition for related path in {
"compilerOptions": {
"baseUrl": ".",
"paths": {
"parse/dist/parse": ["node_modules/@types/parse/index.d.ts"]
}
}
} |
Please fill in this template.
npm test <package to test>.Select one of these and delete the others:
If changing an existing definition:
Changes, in the order of the 4 commits:
npm run prettier -- --write types/parse/**. See https://github.com/DefinitelyTyped/DefinitelyTyped/blob/394b6f5ceedd3acaea0d183f8d2c9b7117c8b3f3/README.md#common-mistakests4.0folder. Tests for the default types are passing from 3.7 to 4.3, and a split at ts4.0 is quire rare in the rest of this repo (even @types/node doesn't have one).no-self-importisn't needed. This meansnode.d.tsimports./indexinstead ofparse.ts3.6with the last changes from the default types folder.Parse.Objectsetandsavemethods. The tricks in use don't appear to be fixing any issue but make code completion difficult. They also break in some cases