Closed jasongwartz closed 2 weeks ago
I'm experiencing the same issue.
It's clear from the usage that an array of both successful and failed responses (resultsInJSONFormat
) is being given as the second argument to the ImportError
constructor:
https://github.com/typesense/typesense-js/blob/7183419ae99e890b09833e24b3a55c5c56b803f4/src/Typesense/Documents.ts#L412-L417
And there is already a test which demonstrates that ImportError.importResults
is intended to be an array:
https://github.com/typesense/typesense-js/blob/7183419ae99e890b09833e24b3a55c5c56b803f4/test/Typesense/Documents.spec.js#L523-L526
The fix for the types in this PR looks good, and I wonder why it hasn't been merged yet?
I would just add that this mistake could have been picked up sooner if the ImportError
class had typings on its constructor arguments. Unfortunately they're implicit any
:
Thank you both for following up on this 👍
I think a better way to approach this is, since the error is thrown, the most important information for the caller is which of the import statements failed. Currently, it will print all of the import responses.
If the thrown error only contains the failed responses, the calling application will have no way to inspect the successful responses. So I would favour retaining the current behaviour. (This would also be a breaking change.)
Thank you both for following up on this 👍
I think a better way to approach this is, since the error is thrown, the most important information for the caller is which of the import statements failed. Currently, it will print all of the import responses.
If the thrown error only contains the failed responses, the calling application will have no way to inspect the successful responses. So I would favour retaining the current behaviour. (This would also be a breaking change.)
Another way to approach this is leaving it to the caller to unwrap it themselves, although that breaks even more things in terms of backwards compatibility. So I think we should leave it as is for the time being then! As soon as @jasongwartz has the type annotations in place, I think we're done
Thank you for the PR!
Published in 2.0.0-1
Change Summary
The docs indicate that the error thrown by
import()
(as in:client.collections(...).documents().import(...)
) contains a keyimportResults
. In my usage, I found that the runtime type of the value ofimportResults
is an array, which seems to contain eitherImportResponseSuccess
orImportResponseFail
. At the moment I'm force-casting to override the type given by the library, but if my experimentation is correct, it'd be good to include the correct type here.PR Checklist