Execute /studio-app/sync/import network request from node process - #1972
Conversation
| return pushArchiveResponseSchema.parse( rawResponse ); | ||
| } | ||
|
|
||
| export function removeTemporaryFile( event: IpcMainInvokeEvent, path: string ) { |
There was a problem hiding this comment.
Renaming this function is a secondary function of this PR. I did it because "temporal" seems like a misnomer in this context.
| } | ||
|
|
||
| export async function exportSiteToPush( | ||
| export async function exportSiteForPush( |
There was a problem hiding this comment.
Renaming this function is also a secondary function of this PR. I did it because I think this clarifies the relationship better between this function and pushArchive.
📊 Performance Test ResultsComparing 366c3cc vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
|
I ran a quick test with a 767MB archive to compare this branch with the latest stable release. The results show significant promise:
|
| const parseResult = z.object( { error: z.string() } ).safeParse( error ); | ||
|
|
||
| if ( parseResult.success ) { | ||
| return { success: false, error: parseResult.data.error }; | ||
| } | ||
|
|
||
| return { success: false, error: 'Unknown error' }; |
There was a problem hiding this comment.
IPC functions that throw in the node process will also throw in the renderer process. The crux is that the error object thrown by the renderer has a different structure, so the most reliable way to pass data back to the renderer is to simply return.
epeicher
left a comment
epeicher
left a comment
There was a problem hiding this comment.
Thanks @fredrikekelund for improving this! I have tested the Push with different scenarios using both partial sync and full sync, and they all have worked as expected. LGTM!
I have just left a non-blocking question out of curiosity
| } | ||
|
|
||
| try { | ||
| await wpcom.req.post( { |
There was a problem hiding this comment.
Is the response of this either a success or an exception in all cases? I was considering getting the response and check the response.success value from it
There was a problem hiding this comment.
wpcom.req throws if the request fails or if the response HTTP status code isn't in the 200-range. Since we also don't care about the response body, I don't think we gain anything by validating it.
There was a problem hiding this comment.
The wpcom.js and wpcom-xhr-request API is a little arcane, but the network requests are ultimately based on superagent, so I looked at the source code to double-check my assumption here.
There was a problem hiding this comment.
wpcom.req throws if the request fails or if the response HTTP status code isn't in the 200-range. Since we also don't care about the response body, I don't think we gain anything by validating it.
Agreed, thanks for clarifying!
…1972) * Run `/studio-app/sync/import` from node process * Improve error handling
2 participants
Related issues
Proposed Changes
Previously, the
/sites/${ remoteSiteId }/studio-app/sync/importAPI request was made from the renderer process. This required the entire site archive to be stored in the renderer process's memory. This isn't necessary, and it's much more efficient (especially for large archives) to stream the file contents from disk instead. This PR implements apushArchiveIPC handler that does just that.Testing Instructions
Pre-merge Checklist