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

Jump to bottom

[chore] Improve current Error extends #1467

Merged
merged 1 commit into from May 22, 2023
Merged

[chore] Improve current Error extends #1467

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
48 changes: 16 additions & 32 deletions pyscriptjs/src/exceptions.ts
View file
Open in desktop
Expand Up @@ -2,15 +2,10 @@ const CLOSEBUTTON = `<svg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 16 16'

type MessageType = 'text' | 'html';

/*
These error codes are used to identify the type of error that occurred.
The convention is:
* PY0 - errors that occur when fetching
* PY1 - errors that occur in config
* Py2 - errors that occur in plugins
* PY9 - Deprecation errors
*/

/**
* These error codes are used to identify the type of error that occurred.
* @see https://docs.pyscript.net/latest/reference/exceptions.html?highlight=errors
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

You are probably not done yet. I'm just thinking out loud while I glance over the PR. Yes, we should describe the error code categories, who are these for here but also add to the docs (arguably more important actually :) )

Copy link
Contributor

Choose a reason for hiding this comment

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

We do have these in the docs under exceptions but I think we may not have all of them.

When I added this docstring the idea was to keep the reference in the code itself so devs can have a look and know which ones to raise.

With that said, I don't think we should remove them from the code :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. That's what I first thought when I saw this change but I'm not too strongly opinionated (only that it should most definitely be in the docs). In general, 2 the options have pros and cons:

  • having the codes here and in the docs means we need to make sure we are always updating in 2 places (which, in my experience, is really prone to error)
  • having the codes only on the docs and having a link to the docs in the code (the info is less obvious to see from the source code but if a user wants, they can open docs)

Between the 2 options, I'm +0 keeping it on the docs only and having a link in the code (because, in most cases, comments can easily get outdated)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the code to point at https://docs.pyscript.net/latest/reference/exceptions.html?highlight=errors which indeed seems like a very good idea

export enum ErrorCode {
GENERIC = 'PY0000', // Use this only for development then change to a more specific error code
FETCH_ERROR = 'PY0001',
Expand All @@ -29,34 +24,27 @@ export enum ErrorCode {
}

export class UserError extends Error {
messageType: MessageType;
errorCode: ErrorCode;
/**
* `isinstance` doesn't work correctly across multiple realms.
* Hence, `$$isUserError` flag / marker is used to identify a `UserError`.
*/
$$isUserError: boolean;

constructor(errorCode: ErrorCode, message: string, t: MessageType = 'text') {
super(message);
this.errorCode = errorCode;
WebReflection marked this conversation as resolved.
Show resolved Hide resolved
constructor(public errorCode: ErrorCode, message: string, public messageType: MessageType = 'text') {
super(`(${errorCode}): ${message}`);
this.name = 'UserError';
this.messageType = t;
this.message = `(${errorCode}): ${message}`;
this.$$isUserError = true;
}
}

export class FetchError extends UserError {
errorCode: ErrorCode;
constructor(errorCode: ErrorCode, message: string) {
super(errorCode, message);
this.name = 'FetchError';
}
}

export class InstallError extends UserError {
errorCode: ErrorCode;
constructor(errorCode: ErrorCode, message: string) {
super(errorCode, message);
this.name = 'InstallError';
Expand All @@ -78,25 +66,21 @@ export function _createAlertBanner(
break;
}

const banner = document.createElement('div');
banner.className = `alert-banner py-${level}`;

if (messageType === 'html') {
banner.innerHTML = message;
} else {
banner.textContent = message;
}
const content = messageType === 'html' ? 'innerHTML' : 'textContent';
const banner = Object.assign(document.createElement('div'), {
className: `alert-banner py-${level}`,
[content]: message,
});

if (level === 'warning') {
const closeButton = document.createElement('button');
const closeButton = Object.assign(document.createElement('button'), {
id: 'alert-close-button',
innerHTML: CLOSEBUTTON,
});
Comment on lines +76 to +79
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't seen this logic before, does this just create the object returned from createElement with some extra attributes (id, innerHTML?)

This seems like a cool trick!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Object.assign returns the target (first argument) after attaching fields/properties via the object(s) that follow such target. In this specific case it makes multiple fields attached at once so that browsers/engine can also optimize when/if necessary in one shot, instead of two or more.

Copy link
Contributor

Choose a reason for hiding this comment

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

TIL :)


closeButton.id = 'alert-close-button';
closeButton.addEventListener('click', () => {
banner.appendChild(closeButton).addEventListener('click', () => {
banner.remove();
});
closeButton.innerHTML = CLOSEBUTTON;

banner.appendChild(closeButton);
}

document.body.prepend(banner);
Expand Down