Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(script-compiler): add error responses #75

Merged
merged 9 commits into from
Jun 22, 2023
Merged

feat(script-compiler): add error responses #75

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4c793fa
feat(script-compiler): add error responses
otariidae Jun 18, 2023
cd14cf6
refactor: add missing error handling
otariidae Jun 18, 2023
1bc8669
feat: unite error types
otariidae Jun 18, 2023
74f6fe2
refactor: use AbortController to remove event listeners
otariidae Jun 19, 2023
f2fcc24
wip: introduce TextlintError
otariidae Jun 19, 2023
1375633
feat: add error response as only error from Textlint worker
otariidae Jun 20, 2023
35097e5
refactor: simplify message handler by using promise chain
otariidae Jun 21, 2023
e181e81
refactor: handle errors in wrapper functions
otariidae Jun 21, 2023
7ea7580
refactor: simplify redundant Promise handling
otariidae Jun 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion packages/@textlint/script-compiler/src/CodeGenerator/worker-codegen.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,16 @@ export type TextlintWorkerCommandResponseFix = {
command: "fix:result";
result: TextlintFixResult;
};
export type TextlintWorkerCommandResponseError = {
id: MessageId | undefined;
command: "error";
error: any;
};
export type TextlintWorkerCommandResponse =
| TextlintWorkerCommandResponseInit
| TextlintWorkerCommandResponseLint
| TextlintWorkerCommandResponseFix;
| TextlintWorkerCommandResponseFix
| TextlintWorkerCommandResponseError;

export const generateCode = async (config: TextlintConfigDescriptor) => {
// macro replacement
Expand Down Expand Up @@ -139,6 +145,12 @@ self.addEventListener('message', (event) => {
command: "lint:result",
result
});
}).catch(error => {
return self.postMessage({
id: data.id,
command: "error",
error
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to make this just a global error?
Ideally, it would be more convenient if users could get it as a default error event in WebWorker.

https://developer.mozilla.org/en-US/docs/Web/API/Worker/error_event

// user can catch the error as global error
worker.addEventListner("error", event => {
  console.error(event);
  console.error(event.error);
});

Current error handling looks like complex.

If make this just a global error, user can just listen "error" event for error handling.

// AbortSignals can be used for event listner
// https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#add_an_abortable_listener
const controller = new AbortController();
const onMessage = (event) => {
  // do somthing
  ontroller.abort(); // unlisten
}
// error handling
const onError = (event) => {
  // do error handling
  controller.abort(); // unlisten
}
worker.addEventListner("message", onMessage, { signal: controller.signal });
worker.addEventListner("error", onError, { signal: controller.signal });

📝 Custom Error may be needed.

class TextlintError extends Error {
  constructor(message, id){
   // https://javascript.info/custom-errors
  }
}

Copy link
Contributor Author

@otariidae otariidae Jun 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great for me! But ErrorEvent.event seems to be always null.
Moreover I found addEventListener("error", ...) can handle thrown Error but cannot handle promise rejection. I am considering another idea based on your review.

Copy link
Contributor Author

@otariidae otariidae Jun 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found error event listener can handle promise rejection in the worker if the worker handle unhandledrejection event and use reportError.

// in the worker
self.addEventListener("unhandledrejection", error => {
    reportError(error)
})

But I haven't found how to handle id through error event listener.

Copy link
Member

@azu azu Jun 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, reportError allow to report ErrorEvent which include custom Error object.

const error = new Error('description');
error.id = "test-1";
self.reportError(new ErrorEvent('textlint error', {
    error : error,
    message : 'description',
}));
image

Technically, it could be written as follows.

class LintError extends Error { }
lintText(...).catch(error => {
  reportError(new ErrorEvent('textlint error', {
    error : new LintError("description", { id }),
    message : 'description',
}))

However, Safari/Node.js/Deno does not support reportError in WebWorker yet.
https://developer.mozilla.org/en-US/docs/Web/API/reportError

Copy link
Contributor Author

@otariidae otariidae Jun 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a wip commit f2fcc24 including debug comments to share the situation.
In this case I got a ErrorEvent with Uncaught #<EventError> message and null error, so reportError with ErrorEvent wrapping TextlintError does not work for me.

スクリーンショット 2023-06-20 021005

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fmm, It may be worker limitation.

image

We can not pass the details of the error in worker via error event.

Copy link
Contributor Author

@otariidae otariidae Jun 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your information.
Another option for less complex error handling I think is that all errors in the worker are handled through worker messages instead of throwing Error or calling reportError. It seems almost practically safe to omit handling of error and messaageerror. I finally understand why other libraries such as comlink have their own error message protocol.
If we want to promise more safety, worker can post error messages when the worker gets error or unhandledrejection on the worker side:

self.addEventListener('error', (error) => {
    // handling all unexpected error on the worker side and notify the client through message
    self.postMessage({
        command: 'error',
        error,
    })
})

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another option for less complex error handling I think is that all errors in the worker are handled through worker messages instead of throwing Error or calling reportError

Yes. it is good that keep it simple.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

implemented in 1375633
Textlint worker handles all possible errors and post error messages. It simplified client-side error handling by omitting error and messageerror event

});
case "fix":
return kernel.fixText(data.text, {
Expand All @@ -153,6 +165,12 @@ self.addEventListener('message', (event) => {
command: "fix:result",
result
});
}).catch(error => {
return self.postMessage({
id: data.id,
command: "error",
error
});
});
default:
console.log("Unknown command: " + data.command);
Expand Down
80 changes: 64 additions & 16 deletions packages/textchecker-element/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,16 +41,40 @@ const generateMessageId = () => crypto.randomUUID();
const createTextlint = ({ worker, ext }: { worker: Worker; ext: string }) => {
const lintText: LintEngineAPI["lintText"] = async ({ text }: { text: string }): Promise<TextlintResult[]> => {
updateStatus("linting...");
return new Promise((resolve, _reject) => {
return new Promise((resolve, reject) => {
const id = generateMessageId();
worker.addEventListener("message", function handler(event) {
const data: TextlintWorkerCommandResponse = event.data;
if (data.command === "lint:result" && data.id === id) {
resolve([data.result]);
worker.removeEventListener("message", handler);
function onMessage(event: MessageEvent<TextlintWorkerCommandResponse>) {
const data = event.data;
if ("id" in data && data.id === id) {
if (data.command === "error") {
reject(data.error);
updateStatus("failed to lint");
} else if (data.command === "lint:result") {
resolve([data.result]);
updateStatus("linted");
}
worker.removeEventListener("message", onMessage);
worker.removeEventListener("messageerror", onMessageError);
worker.removeEventListener("error", onError);
}
updateStatus("linted");
});
}
function onMessageError(event: MessageEvent<any>) {
reject(event.data);
updateStatus("failed to lint");
worker.removeEventListener("message", onMessage);
worker.removeEventListener("messageerror", onMessageError);
worker.removeEventListener("error", onError);
}
function onError(event: ErrorEvent) {
reject(event);
updateStatus("failed to lint");
worker.removeEventListener("message", onMessage);
worker.removeEventListener("messageerror", onMessageError);
worker.removeEventListener("error", onError);
}
worker.addEventListener("message", onMessage);
worker.addEventListener("messageerror", onMessageError);
worker.addEventListener("error", onError);
return worker.postMessage({
id,
command: "lint",
Expand All @@ -67,16 +91,40 @@ const createTextlint = ({ worker, ext }: { worker: Worker; ext: string }) => {
message?: TextlintMessage;
}): Promise<TextlintFixResult> => {
updateStatus("fixing...");
return new Promise((resolve, _reject) => {
return new Promise((resolve, reject) => {
const id = generateMessageId();
worker.addEventListener("message", function handler(event) {
const data: TextlintWorkerCommandResponse = event.data;
if (data.command === "fix:result" && data.id === id) {
resolve(data.result);
worker.removeEventListener("message", handler);
function onMessage(event: MessageEvent<TextlintWorkerCommandResponse>) {
const data = event.data;
if ("id" in data && data.id === id) {
if (data.command === "error") {
reject(data.error);
updateStatus("failed to fix");
} else if (data.command === "fix:result") {
resolve(data.result);
updateStatus("fixed");
}
worker.removeEventListener("message", onMessage);
worker.removeEventListener("messageerror", onMessageError);
worker.removeEventListener("error", onError);
}
updateStatus("fixed");
});
}
function onMessageError(event: MessageEvent<any>) {
reject(event.data);
updateStatus("failed to fix");
worker.removeEventListener("message", onMessage);
worker.removeEventListener("messageerror", onMessageError);
worker.removeEventListener("error", onError);
}
function onError(event: ErrorEvent) {
reject(event);
updateStatus("failed to fix");
worker.removeEventListener("message", onMessage);
worker.removeEventListener("messageerror", onMessageError);
worker.removeEventListener("error", onError);
}
worker.addEventListener("message", onMessage);
worker.addEventListener("messageerror", onMessageError);
worker.addEventListener("error", onError);
return worker.postMessage({
id,
command: "fix",
Expand Down
12 changes: 9 additions & 3 deletions packages/textchecker-element/src/attach-to-text-area.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,9 +155,15 @@ export const attachToTextArea = ({
// dismiss card before update annotations
// dismissCards();
const text = textAreaElement.value;
const results = await lintEngine.lintText({
text
});
let results;
try {
results = await lintEngine.lintText({
text
});
} catch (e) {
debug("lint error", e);
results = [] as const;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Errors should be handled by the caller.

update().catch(error => ...) or create some wrapper for logging.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

implemented in e181e81
But I am not so much familiar with layered error handling technique so please review if it may be wrong for you.

debug("lint results", results);
const updateText = async (newText: string, card: TextCheckerCard) => {
const currentText = textAreaElement.value;
Expand Down
70 changes: 56 additions & 14 deletions packages/webextension/app/scripts/background/textlint.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,36 @@ export const createTextlintWorker = (script: Script) => {
const defaultWorker = new Worker(workerUrl);
const workerRef = createWorkerRef(defaultWorker);
const lintText = async ({ text, ext }: { text: string; ext: string }): Promise<TextlintResult[]> => {
return new Promise((resolve, _reject) => {
return new Promise((resolve, reject) => {
const id = generateMessageId();
workerRef.current.addEventListener("message", function handler(event) {
const data: TextlintWorkerCommandResponse = event.data;
if (data.command === "lint:result" && data.id === id) {
resolve([data.result]);
workerRef.current.removeEventListener("message", handler);
function onMessage(event: MessageEvent<TextlintWorkerCommandResponse>) {
const data = event.data;
if ("id" in data && data.id === id) {
if (data.command === "error") {
reject(data.error);
} else if (data.command === "lint:result") {
resolve([data.result]);
}
workerRef.current.removeEventListener("message", onMessage);
workerRef.current.removeEventListener("messageerror", onMessageError);
workerRef.current.removeEventListener("error", onError);
}
});
}
function onMessageError(event: MessageEvent<any>) {
reject(event.data);
workerRef.current.removeEventListener("message", onMessage);
workerRef.current.removeEventListener("messageerror", onMessageError);
workerRef.current.removeEventListener("error", onError);
}
function onError(event: ErrorEvent) {
reject(event);
workerRef.current.removeEventListener("message", onMessage);
workerRef.current.removeEventListener("messageerror", onMessageError);
workerRef.current.removeEventListener("error", onError);
}
workerRef.current.addEventListener("message", onMessage);
workerRef.current.addEventListener("messageerror", onMessageError);
workerRef.current.addEventListener("error", onError);
return workerRef.current.postMessage({
id,
command: "lint",
Expand All @@ -85,15 +106,36 @@ export const createTextlintWorker = (script: Script) => {
ext: string;
message?: TextlintMessage;
}): Promise<TextlintFixResult> => {
return new Promise((resolve, _reject) => {
return new Promise((resolve, reject) => {
const id = generateMessageId();
workerRef.current.addEventListener("message", function handler(event) {
const data: TextlintWorkerCommandResponse = event.data;
if (data.command === "fix:result" && data.id === id) {
resolve(data.result);
workerRef.current.removeEventListener("message", handler);
function onMessage(event: MessageEvent<TextlintWorkerCommandResponse>) {
const data = event.data;
if ("id" in data && data.id === id) {
if (data.command === "error") {
reject(data.error);
} else if (data.command === "fix:result") {
resolve(data.result);
}
workerRef.current.removeEventListener("message", onMessage);
workerRef.current.removeEventListener("messageerror", onMessageError);
workerRef.current.removeEventListener("error", onError);
}
});
}
function onMessageError(event: MessageEvent<any>) {
reject(event.data);
workerRef.current.removeEventListener("message", onMessage);
workerRef.current.removeEventListener("messageerror", onMessageError);
workerRef.current.removeEventListener("error", onError);
}
function onError(event: ErrorEvent) {
reject(event);
workerRef.current.removeEventListener("message", onMessage);
workerRef.current.removeEventListener("messageerror", onMessageError);
workerRef.current.removeEventListener("error", onError);
}
workerRef.current.addEventListener("message", onMessage);
workerRef.current.addEventListener("messageerror", onMessageError);
workerRef.current.addEventListener("error", onError);
return workerRef.current.postMessage({
id,
command: "fix",
Expand Down