Make requests more reliable and decrease hang state chance #82
+1,180
−1,461
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
WorldThirteen
commented
Oct 1, 2021
| @@ -12,7 +12,7 @@ import { crypto, pkijs, asn1js } from "../crypto"; | |||
|
|
|||
| (global as any)["DOMParser"] = DOMParser; | |||
| (global as any)["XMLSerializer"] = XMLSerializer; | |||
| XAdES.Application.setEngine("@peculiar/webcrypto", crypto); | |||
| XAdES.Application.setEngine("@peculiar/webcrypto", crypto as Crypto); | |||
There was a problem hiding this comment.
Required since it seems Webcrypto's Crypto interface is slightly different from native Crypto in some stage detail:
src/formats/eutl.ts:8:52 - error TS2345: Argument of type 'import("/Users/worldthirteen/Documents/Projects/pv/tl-create/node_modules/@peculiar/webcrypto/index").Crypto' is not assignable to parameter of type 'Crypto'.
Types of property 'getRandomValues' are incompatible.
Type '<T extends Uint8Array | Int8Array | Int16Array | Int32Array | Uint16Array | Uint32Array | Uint8ClampedArray | Float32Array | Float64Array | DataView | null>(array: T) => T' is not assignable to type '<T extends ArrayBufferView | null>(array: T) => T'.
Types of parameters 'array' and 'array' are incompatible.
Type 'T' is not assignable to type 'Uint8Array | Int8Array | Int16Array | Int32Array | Uint16Array | Uint32Array | Uint8ClampedArray | Float32Array | Float64Array | DataView | null'.
Type 'ArrayBufferView | null' is not assignable to type 'Uint8Array | Int8Array | Int16Array | Int32Array | Uint16Array | Uint32Array | Uint8ClampedArray | Float32Array | Float64Array | DataView | null'.
Type 'ArrayBufferView' is not assignable to type 'Uint8Array | Int8Array | Int16Array | Int32Array | Uint16Array | Uint32Array | Uint8ClampedArray | Float32Array | Float64Array | DataView | null'.
Type 'ArrayBufferView' is missing the following properties from type 'DataView': getFloat32, getFloat64, getInt8, getInt16, and 17 more.
Type 'T' is not assignable to type 'DataView'.
Type 'ArrayBufferView | null' is not assignable to type 'DataView'.
Type 'null' is not assignable to type 'DataView'.
8 XAdES.Application.setEngine("@peculiar/webcrypto", crypto);
so I was forced to add this as.
There was a problem hiding this comment.
The problem is that TS changes WebCrypto interfaces from version to version. We need to upgrade TS package version for all of our crypto modules
| @@ -12,7 +12,8 @@ | |||
| "importHelpers": true, | |||
| "esModuleInterop": true, | |||
| "forceConsistentCasingInFileNames": true, | |||
| "skipLibCheck": true | |||
| "skipLibCheck": true, | |||
| "useUnknownInCatchVariables": false, | |||
There was a problem hiding this comment.
Since the code contains catch and the error type doesn't correctly validate, I was forced to disable this check.
microshine
approved these changes
Oct 4, 2021
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.
There was a problem with the
sync-requestlibrary, it contains bugs and is no longer actively maintained.The most critical bug for tl_creaate use-case was hanging connection and useless timeout param which leads
tl-createto hang forever if some server isn't responding.To make network requests more reliable I used
node-fetchinstead ofsync-requestwhich forces me to refactor sync flow into async/await style.The user agent string was slightly modified since I had a guess (just guess based on some experiments) that some remote servers might rate limiting/filtering by user-agent, so I pick not so common (random one doesn't give a positive effect).
There were also some tradeoffs in the code I used to make (probably temporary, will be commented in this PR near the code itself.)
@microshine, @rmhrisk, please review.