Closed wxfred closed 5 months ago
@wxfred I've just quickly checked but as far as I could see, all sendRequest
calls are wrapped in try/catch with console.error as the error handler. Can you share the stack trace of the crash?
https://github.com/ueberdosis/hocuspocus/blob/main/packages/extension-webhook/src/index.ts#L122
@janthurau Thanks for your prompt, I located the error, the original code didn't add async/await to axios, sendRequest returns a promise, and here are my changes
async onChange(data) {
if (!this.configuration.events.includes(Events.onChange)) {
return;
}
const save = async () => { // add async
try {
await this.sendRequest(Events.onChange, { // add await
document: this.configuration.transformer.fromYdoc(data.document),
documentName: data.documentName,
context: data.context,
requestHeaders: data.requestHeaders,
requestParameters: Object.fromEntries(data.requestParameters.entries()),
});
}
catch (e) {
console.error(`Caught error in extension-webhook: ${e}`);
}
};
if (!this.configuration.debounce) {
return save();
}
this.debounce(data.documentName, save);
}
BTW, I found that other hooks actually have async/await, the miss is only in onChange.
uff. Thanks for reporting this! Will push a fix right away.
Description If my nestjs server returns a 400 status code response to the onChange webhook, the hocuspocus will crash.
I see there is no catch clause in hocuspocus-webhook.esm.js
Is it suitable to add a catch like below?
And, sendRequest uses axios to send requests, it seems all 2xx status code will be treated as a success, while others will throw a exception which crashes the entire server. The line 109 shows
Maybe, don't hard code response.status !== 200 will be nice, because POST responses of nestjs will contain a 201 status code by default and axios considers it as a success.
Summary
That's my own adverise, welcome to discuss.
Environment?