use IPC bridge for fs relating to responses#8311
use IPC bridge for fs relating to responses#8311jackkav wants to merge 9 commits intoKong:developfrom
Conversation
jackkav
changed the title
| export const readCurlResponse = async (options: { bodyPath?: string; bodyCompression?: Compression }) => { | ||
| const readFailureMsg = '[main/curlBridgeAPI] failed to read response body message'; | ||
| const bodyBufferOrErrMsg = getBodyBuffer(options, readFailureMsg); | ||
| // TODO(jackkav): simplify the fail msg and reuse in other getBodyBuffer renderer calls | ||
|
|
||
| if (!bodyBufferOrErrMsg) { | ||
| return { body: '', error: readFailureMsg }; | ||
| } else if (typeof bodyBufferOrErrMsg === 'string') { | ||
| if (bodyBufferOrErrMsg === readFailureMsg) { | ||
| return { body: '', error: readFailureMsg }; | ||
| } | ||
| return { body: '', error: `unknown error in loading response body: ${bodyBufferOrErrMsg}` }; | ||
| } | ||
|
|
||
| return { body: bodyBufferOrErrMsg.toString('utf8'), error: '' }; | ||
| }; | ||
| export const getBodyBuffer = ( | ||
| response?: { bodyPath?: string; bodyCompression?: Compression }, | ||
| readFailureValue?: string, | ||
| ): Buffer | string | null => { | ||
| if (!response?.bodyPath) { | ||
| // No body, so return empty Buffer | ||
| return Buffer.alloc(0); | ||
| } | ||
| try { | ||
| const rawBuffer = fs.readFileSync(response?.bodyPath); | ||
| if (response?.bodyCompression === 'zip') { | ||
| return zlib.gunzipSync(rawBuffer); | ||
| } else { | ||
| return rawBuffer; | ||
| } | ||
| } catch (err) { | ||
| console.warn('Failed to read response body', err.message); | ||
| return readFailureValue === undefined ? null : readFailureValue; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Looks great!
Not sure if there's anything better we can do for now for the useEffect.
In react 19 we can replace this with the use function or we could come up with a way to read the data using the defer api from react-router but that will also be simpler in v7.
Another idea could be to use file://{PATH} urls and get read this using fetch and the electron api protocol and remove the need for the bridge api but it wouldn't change the ergonomics of how we read this
|
2 participants
Motivation: it was identified in an earlier PR to reduce the complexity of the getBodyBuffer and some of the workarounds build to wrap it.
Approach: remove it and any wrappers, replace with fs readFile and a main bridge in the renderer.
TODO:
future work:
Takeaways:
given these takeaways its worth considering what would happen in the reset of the system given an oversized response.
Option 1: continue to ignore these cases
Option 2: build a file reading abstraction in main which avoids loading large files into chromium, by offering to open to file in another application.