feat: DH-19897: Handle missing jsapi/dh-internal.js files on core workers #2501
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2501 +/- ##
========================================
Coverage 44.67% 44.67%
========================================
Files 762 762
Lines 42617 42617
Branches 10710 10906 +196
========================================
Hits 19038 19038
+ Misses 23568 23522 -46
- Partials 11 57 +46
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
This is a pretty hacky solution; my only real issue is making this a breaking change, when we really should try our best not to be breaking APIs unnecessarily.
We could convert the function to take an object, but also still accept the old parameter list for example; or somehow do this so it doesn't require a breaking change... perhaps the post download transform could be used, to handle an error scenario and return content instead.
@mofojed I think I can make it such that the api contract doesn't have to change The 1 piece in |
| download, | ||
| storageDir, | ||
| targetModuleType, | ||
| handleErrorsInPostDownload, |
There was a problem hiding this comment.
@mofojed This isn't my favorite, but it should get rid of the breaking change.
Alternative would be to check the number of args in the PostDownloadTransform function and if it expects 3 args assume it wants to handle errors. This may still technically be a breaking change since someone could have passed in a 3 arg function already, and this would change the behavior. This would be a crazy edge case though IMO.
There was a problem hiding this comment.
Eh this is alright but the flag isn't being used correctly right now. You could just add an errorTransform specifically rather than the download transform, and use the appropriate transform for the situation.
| // know whether it's present or not. Treat 404s as empty content | ||
| // to make things compatible with both configurations. | ||
| if (error instanceof HttpError && error.statusCode === 404) { | ||
| return ''; |
There was a problem hiding this comment.
This should really be the only behavior change from the consumers perspective, and I don't think there's any real scenarios where this file would 404 when a server is up and running except our future scenario when we remove the file.
| if (typeof download === 'function' && handleErrorsInPostDownload === true) { | ||
| // If we are to handle errors in post download, we need to get both | ||
| // resolved and rejected Promises. | ||
| const settledResults = await Promise.allSettled(downloadPromises); | ||
|
|
||
| contents = settledResults.map((result, i) => | ||
| result.status === 'fulfilled' | ||
| ? // Post-download transform | ||
| download(serverPaths[i], result.value) | ||
| : // Post-download transform that also handles errors | ||
| download(serverPaths[i], '', result.reason) | ||
| ); | ||
| } else { | ||
| contents = await Promise.all(downloadPromises); |
There was a problem hiding this comment.
You're only using the download transform if handleErrorsInPostDownload is true right now.
| (serverPath, content) => { | ||
| if (serverPath === 'jsapi/dh-core.js') { | ||
| (serverPath, content, error) => { | ||
| if (serverPath === DH_CORE_MODULE) { |
There was a problem hiding this comment.
Should be re-throwing the error here.
Probably should just have a case for the error case originally, e.g.
if (error != null) {
if (serverPath === DH_INTERNAL_MODULE && error instanceof HttpError && error.statusCode === 404) {
// This is the one error we want to ignore
return '';
}
throw error;
}
Then below is just replacing the content in the success cases.
| download, | ||
| storageDir, | ||
| targetModuleType, | ||
| handleErrorsInPostDownload, |
There was a problem hiding this comment.
Eh this is alright but the flag isn't being used correctly right now. You could just add an errorTransform specifically rather than the download transform, and use the appropriate transform for the situation.
DH-19897: Handle missing jsapi/dh-internal.js files on core workers
I added a flag to resolve 404s as empty content instead of throwing an error. 404s were already differentiated from other errors that get retry attempts, so it was a simple change.
Testing
I tested locally by renaming
jsapi/dh-internal.jsin theloadModulescall tojsapi/dh-internal2.jsto mimick a 404. Confirmed empty content file gets created in VS Code extension.I also tested stopping the server which produces a different error that invokes the retry mechanism instead of a 404. Worst case scenario, if we ever get out of sorts, restarting VS Code will delete the tmp download folder and retry everything again.