refactor(ts-migration): migrates ndjsonstream to typescript #2053
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
seaerchin
force-pushed
the
refactor/migrate-ndjsonstream
branch
from
3ad5c64
to
a3c6885
Compare
seaerchin
requested review from
mantariksh,
karrui,
chowyiyin,
liangyuanruo,
tshuli and
yong-jie
mantariksh
reviewed
Jun 3, 2021
karrui
requested changes
Jun 7, 2021
karrui
reviewed
Jun 7, 2021
seaerchin
changed the base branch from
refactor/process-decrypted-content
to
refactor/extract-admin-submissions
seaerchin
changed the base branch from
refactor/extract-admin-submissions
to
refactor/process-decrypted-content
mantariksh
approved these changes
Jun 8, 2021
|
Will be reopening into |
seaerchin
added a commit
that referenced
this pull request
Jun 9, 2021
* feat(ndjsonstream): refactored ndjsonstream and removed old js version * refactor(submissions.client.factory): updated callsite to use new version of ndjsonstream * revert(ndjsonstream): adds data_buf back to prevent regression * fix(ndjsonstream): fixed errorneous initial conditional * fix(ndjsontream): fixed insidious bug causing occasional off by 1 * revert(ndjsonstream): reverts slice/map to for loops * revert(ndjsonstream): reverts single line return
4 tasks
seaerchin
added a commit
that referenced
this pull request
Jun 9, 2021
…2111) * refactor(process-decrypted-content): ported over process-decrypted-content to typescript * refactor(public/modules/forms/admin): changed callsites for process-decrypted-content * refactor(process-decrypted-content): updated type predicate * refactor(process-decrypted-content): inlined typeguard for neatness * refactor(ts-migration): migrates ndjsonstream to typescript (#2053) * feat(ndjsonstream): refactored ndjsonstream and removed old js version * refactor(submissions.client.factory): updated callsite to use new version of ndjsonstream * revert(ndjsonstream): adds data_buf back to prevent regression * fix(ndjsonstream): fixed errorneous initial conditional * fix(ndjsontream): fixed insidious bug causing occasional off by 1 * revert(ndjsonstream): reverts slice/map to for loops * revert(ndjsonstream): reverts single line return
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part 2 of extracting admin downloads
Problem
ndjsonstreamwas previously written in javascript, which lacked type-safety. this meant that consumers did not know the concrete types which makes it hard to reliably use the stream and to troubleshoot.Solution
Notes
try/catchblock for ndjsonstream so thatenqueuedoes not fail. however,enqueueonly throws onTypeError, where the controller is not aReadableDefaultStream. i'm currently unsure if this would ever happen (for once, the mozilla docs are unclear about where this controller originates from) but i left it in because it's additional safetyBreaking Changes