[chore] Improve current Error extends

[WebReflection]

Description

While digging into the dance around fetch files, I stumbled upon this file which looked a bit weird to me so I've decided to somehow apply the "boy-scout rule" and change it.

Changes

• removed badly formatted comments
• removed unnecessary comments + added one that made me think more than I should've (errorCode)
• used TypeScript public fields to simplify code and avoid redundant info / details
• used inline element creation instead of prop after prop assignments
• used appendChild returned value to attach a listener

Checklist

• All tests pass locally
• I have updated docs/changelog.md
• I have created documentation for this(if applicable)

[WebReflection]

Personal Thoughts

I have questioned myself the meaning of UserError as everything extends it ... and maybe answered my questions in the process, but:

• are fetch or install errors really always users related?
• in which other case an error is not a user one?

In short, I think we could've used just new Error and maybe attach the ErrorCode at the beginning of its message:

throw new Error(`[${ErrorCode.value}] ${message}`)

The ErrorCode could be a map with messageType details in it, because I doubt same error code would sometimes show HTML and sometimes just text + I really didn't get the logMessage boolean value as that seems to be a shared global/module property to look at, or better, in not a single _createAlertBanner I've seen it passed as flag ... it looks there either by accident or left behind some previous refactoring.

[fpliger]

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 :) )

[FabioRosado]

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 :)

[fpliger]

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)

[WebReflection]

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

[FabioRosado]

For context we raise UserError for the errors that we want to show a banner on the page, so far most of these tend to be user errors :)

We used to have places where we were raising UserErrors with html formatting and others where we just send text that's why we used to have the param to do one or the other.

I think the same is true for the logMessage Boolean but it's been a while since I wrote that code

[WebReflection]

@FabioRosado

For context we raise UserError for the errors that we want to show a banner on the page

thanks for clarifying that part but it's not clear to me how these errors, not shown to users, not thrown or added to the message for developers, can help anyone filing issues so that was my wonder and your answer makes half sense as in: if the error has a meaning, beisde the message, any issue or debugging session should instantly understand that meaning via the error code ... this is fairly common in most software but not here and I think we can improve there for everyone sake, filed issues + us debugging.

edit

We actually indeed show the error code as part of the message so my findings were wrong.

[FabioRosado]

Oh wait we aren't sending the error code? When I wrote this logic we were attaching both the error code and the error message to both the banner and the console something like:

PY1001: everything is exploding please don't.

Then in our docs under api exceptions we have the reference for these codes and some have suggestions for example this one when we can't install a package

If I'm not mistaken we were adding the link to this reference in the error message when we try to install a package.

how these errors, not shown to users, not thrown or added to the message for developers

Did this change as well? Our codebase used to throw UserErrors like:

Throw UserError(ErrorCode.blah, "everything is exploding, ohnoes D:")

And then we used to have a general exception handler that would catch the UserError and handle it with creating the banner and logging the message if the flag was passed.

But as I mentioned it's been a while since I looked at the code and I know a lot had changed haha

I'm on holidays at the moment so it's a bit hard for me to look at the codebase with time to see if we still do all these things 😄

[WebReflection]

@FabioRosado

And then we used to have a general exception handler

I need to dig into that too then, my apology for not tracking this better/further

I'm on holidays at the moment ...

so enjoy your holidays, there's no rush whatsoever around this chore related change and no need for you to dig into it but thanks for your answers and pointers so far!

[fpliger]

Ok, just to make sure I'm reading the intent correctly.

1. We all agree that we want to always show errorCode + message. Right?
2. @FabioRosado when you say:

For context we raise UserError for the errors that we want to show a banner on the page

Do you mean that you need to raise UserError to show a banner on the page (and that's the main reason we use it) or that UserError is just one of the types of errors that show a banner on the page?

[FabioRosado]

Ok, just to make sure I'm reading the intent correctly.

1. We all agree that we want to always show errorCode + message. Right?
2. @FabioRosado when you say:

For context we raise UserError for the errors that we want to show a banner on the page

Do you mean that you need to raise UserError to show a banner on the page (and that's the main reason we use it) or that UserError is just one of the types of errors that show a banner on the page?

That is correct we are catching UserError and use that to create the banner. This is only for error banners, warnings don't usually work that way, but I think we can pass a level arg or maybe we talked about it when I created the banners 😄

[WebReflection]

@FabioRosado @fpliger this MR is finally green again after Jeff pinned the version of our pytest dependency ... feel free to approve or ask for changes as all points have been addressed now, thank you!

[FabioRosado]

@WebReflection Thanks for your work here and also patience as I tried to understand what had changed in the codebase haha

I've added a comment which is mostly a question for myself, the PR looks great 😄

[FabioRosado]

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!

[WebReflection]

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.

[fpliger]

TIL :)

[fpliger]

That is correct we are catching UserError and use that to create the banner. This is only for error banners, warnings don't usually work that way, but I think we can pass a level arg or maybe we talked about it when I created the banners 😄

Yeah, would be better to make it explicit. UserError doesn't give any hint that "this type of Error will show in the banner". Maybe adding a base PyScriptError that add level and have UserError and our other error classes inherit from it is the right thing to do here...

[hoodmane]

Though UserError has a connotation that it's the "user"'s fault and PyScriptError also has a connotation that it's a "pyscript" internal error fault but it could be either. I wonder if there is a good way to refer to managed errors that come out of expected places but could be due to one of:

1. a PyScript internal bug
2. a bug in the user's code
3. a managed error that the user might want to catch

Though I don't think type 3 exists.

I think inheritance is a bad way to handle this situation because it's not very flexible. Maybe a good thing to do would be:

• any error that makes it to the top of the stack gets a banner
• We set a userError property on the error to indicate that it is not an internal error
• if the userError property is not set we say "internal PyScript error report to maintainers" and if it is set we don't say that

This way, if we catch an error, we are free to mark it as a userError and rethrow it (this will give more detailed stack traces) or make a different error and we're not constrained by the decision of whether to make a banner or not.

This is basically the mechanism that Pyodide uses except that errors are considered to be a user error by default and if they are calculated to be internal errors then a special marker is placed on them.

[fpliger]

Yeah, good point. The main point I was trying to convey is that the type of error shouldn't necessarily be the only way you decide if an error makes it to the banner. Error types indicate the category of the errors and where these are displayed is a different [yet related] matter.