fix(core): type definition of Dataset.reduce #2774
Conversation
halvko
changed the title
|
Idk how the bad main signature slipped past me, but it's fixed now |
|
The type errors are caused by missing import in that test -import { SitemapRequestList } from '@crawlee/core';
+import { SitemapRequestList, type Request } from '@crawlee/core';I'll adjust the CI so it fails when there are type errors in the tests, I thought we already do that, but apparently not 🙃 |
|
If you are fixing that on upstream, shall I rebase?
…On Tue, Dec 17, 2024, at 13:13, Martin Adámek wrote:
The type errors are caused by missing import in that test
-import { SitemapRequestList } from ***@***.***/core';
+import { SitemapRequestList, type Request } from ***@***.***/core';
I'll adjust the CI so it fails when there are type errors in the tests, I thought we already do that, but apparently not 🙃
—
Reply to this email directly, view it on GitHub <#2774 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAS2UUSD3NN5DCXLRMVHLYT2GAIPTAVCNFSM6AAAAABTWIZDQ2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNBYGI4TMNBSGM>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
Yeah I can do that now, I hope there won't be more type issues in the tests :D |
|
Fixed the eslint errors. I question if it makes sense to separate overload signatures with newlines, but I've done it. |
|
You also need to duplicate the jsdoc on all the overloads, otherwise it wont be part of the generated API docs. This part sucks hard I its the main reason why I always try to not use overloads in the first place 🙃 |
This might take some time, there were a few errors and one of them was caused by the fact that we had strictNullChecks disabled for tests, and enabling that yields 300+ errors :D |
|
When I have fixed the documentation on this, would it be possible to merge before fixing all of the current test issues? It's not this change introducing the problems, right? |
|
As long as the CI is passing, sure. But I will most likely finish this during the afternoon, I just need to add a lot of |
|
I wonder if we really need the overloads, why can't we just mark the second parameter as optional? |
|
Btw we actually do have an existing test for the reduce method, but because of the disabled |
|
That was my first idea too, but the generic is inferred to be the type `undefined` if options are provided. It would also not allow us to guarantee the existence of the return value when memo is provided.
…On Tue, Dec 17, 2024, at 15:37, Martin Adámek wrote:
I wonder if we really need the overloads, why can't we just mark the second parameter as optional?
—
Reply to this email directly, view it on GitHub <#2774 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAS2UURDKT2VXXO3JILQEDT2GAZMJAVCNFSM6AAAAABTWIZDQ2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNBYGYZTCOJQGM>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
This seems like a case of “it worked on my computer”. If possible, can we take this elsewhere and see what goes wrong?
…On Tue, Dec 17, 2024, at 15:49, Martin Adámek wrote:
Btw we actually do have an existing test for the reduce method, but because of the disabled `strictNullChecks` in tests, it was passing just fine on type-level too. And I can see the inference is completely broken too, for both cases, and it feels like the overloads are not helping with that either, I think we will need to use `NoInfer` in them.
—
Reply to this email directly, view it on GitHub <#2774 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAS2UUUQ6B7HAACMF45C7432GA2WVAVCNFSM6AAAAABTWIZDQ2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNBYGY2TGNBUHE>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
The checks are enabled here #2776, I've added some |
BREAKING CHANGE: Dataset.request return type now correctly reflects that `undefined` may be returned
if called on an empty Dataset without the `memo` argument. As calling it without a `memo` argument
already lead to an errorinous type error, users probably already have type casts around it in this
situation, but if not, the new case can be ignored by asserting that there will be a value.
Before:
const res = dataset.reduce(
async (memo, item, index) => {
// example impl
return item;
}
);
After:
const res = dataset.reduce(
async (memo, item, index) => {
// example impl
return item;
}
)!;
Note that the original snippet was already rejected by TypeScript for calling reduce with too few
arguments. The workaround to this was to pass `undefined` as the second argument, making
TypeScript infer the type of `res` as `undefined`. This can be "fixed" in one of two ways:
1. In the DatasetReducer, casting the memo argument and the result of reduce to `Data`.
- In this case no change is neccesary to the user code
2. Calling reduce with the memo argument `undefined as Data`
- In this case the migration is neccesary
@vladfrangu found a missing '?' in the signature
test/core/storages/dataset.test.ts
Outdated
| return memo.concat(item); | ||
| }, | ||
| [], | ||
| new Array(), |
There was a problem hiding this comment.
this is actually fixing something? how is [] different from new Array()?
There was a problem hiding this comment.
Sorry, this should have gone here: #2774 (comment)
|
Yes, `new Array()` is inferred to be an `any[]` when no other type information is available, and `[]` is inferred to be `never[]` when no other type information is available. As the type of the closure depends on the type of `memo`, typescript is unable to use it’s implementation to refine the type of `memo`.
I didn’t check if type annotating the return value would also fix the issue.
…On Sat, Dec 21, 2024, at 17:44, Martin Adámek wrote:
***@***.**** commented on this pull request.
In test/core/storages/dataset.test.ts <#2774 (comment)>:
> return memo.concat(item);
},
- [],
+ new Array(),
this is actually fixing something? how is `[]` different from `new Array()`?
—
Reply to this email directly, view it on GitHub <#2774 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAS2UUQNBWBLAU7YKLFXWXD2GWLHTAVCNFSM6AAAAABTWIZDQ2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKMJYGY4DOOJYGQ>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
Ok, I thought it will be something like that.
Mostly likely yes. Since we disallow array constructor in eslint, it would be a better solution than adding eslint ignore comments. |
It didn't - we can instead solve it by doing a type cast on the input memo |
|
This smells of either bad API design or type definition but it should be noted that |
|
Yeah, I just wanted to point out the same, you are the one to provide a correct type in the initializer, just like in the array method. TS will see |
|
All green, thanks! |
Fixes #2773
I'm seeing type errors and test failures in
./test/core/sitemap_request_list.test.tsboth before and after my change.This PR currently doesn't add tests to check that types resolve correctly. In the
reduce() uses first value as memo if no memo is providedtest, the result from reduce was previously inferred to beany, and is now inferred to beDictionary. Adding a type annotation doesn't help catch this change as implicit any is allowed.