Skip to content
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

feat: add support for exporting documents with "inconsistent" cursor #12

Merged
merged 1 commit into from
Jul 2, 2024

Conversation

sgulseth
Copy link
Member

@sgulseth sgulseth commented Jun 26, 2024

A bit awkward implementation, but we need to handle multiple streams when a cursor is sent by the server. This is also a need code path, so wont affect exports without the flag

src/validateOptions.js Outdated Show resolved Hide resolved
@sgulseth sgulseth requested a review from a team June 26, 2024 12:25
@sgulseth sgulseth force-pushed the feat/add-cursor-support branch from 4871b9d to af0e2b7 Compare July 2, 2024 10:41
@sgulseth sgulseth requested review from atombender and judofyr July 2, 2024 10:43
return
}
} catch (err) {
// Ignore JSON parse errors
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that wise? Smells like the sort of thing that can cause hard-to-figure-out Node.js problems. Better to be strict in such situations; the stream should not have parse errors.

Copy link
Member Author

@sgulseth sgulseth Jul 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can move the catch into JSON parse only, it's only if it's sending something that isn't json, then the error should be handled by a later consumer. Added a comment

src/getDocumentCursorStream.js Outdated Show resolved Hide resolved
src/constants.js Outdated Show resolved Hide resolved
@sgulseth sgulseth force-pushed the feat/add-cursor-support branch from af0e2b7 to 646984e Compare July 2, 2024 12:50
@sgulseth sgulseth force-pushed the feat/add-cursor-support branch from 646984e to 5679759 Compare July 2, 2024 12:52
@sgulseth sgulseth requested a review from atombender July 2, 2024 12:52
Copy link
Member

@atombender atombender left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Not sure I am competent to review the stream stuff since it's been a long time since I used it, but it looks good on first glance.

@sgulseth sgulseth merged commit 4294be0 into main Jul 2, 2024
3 checks passed
@sgulseth sgulseth deleted the feat/add-cursor-support branch July 2, 2024 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants