marko-js / marko

A declarative, HTML-based language that makes building web apps fun
https://markojs.com/
MIT License
13.35k stars 643 forks source link

`<await-error>` calls `console.error` #652

Closed runeh closed 4 years ago

runeh commented 7 years ago

Bug Report

Context

When using <await-error> to deal with rejected promises, marko calls console.error with the error. See https://github.com/marko-js/marko/blob/master/taglibs/async/await-tag.js#L111 .

For us this has two consequences:

Expected Behavior

<await-error> tag does not cause console.error to be called.

Actual Behavior

<await-error> tag does causes console.error to be called.

Possible Fix

Having this logging can be useful during dev. Maybe ut could be behind a flag or something, so you can pass silent-error=true to the <await> tag or similar?

Your Environment

Steps to Reproduce

  1. Load the code from https://gist.github.com/runeh/7adf1873b45ffa9d0b6723da4e410f4c into http://markojs.com/try-online/
  2. Look at output in error console
patrick-steele-idem commented 7 years ago

Putting the logging behind a flag/attribute is definitely an option and I am not opposed to that. However, if you need that level of control, I would recommend not using the <await-error> tag and, instead, make sure there the promise is not rejected by providing your own .catch(). For example:

$ var userInfoPromise = getUserInfo()
    .catch((err) => {
        return {
            error: err
        }
    });

<await(userInfo from userInfoPromise)>
    <if(userInfo.error)>
        We are sorry, we were unable to retrieve the user information.
    </if>
    <else>
        Hello ${userInfo.firstName}!
    </else>
</await>

Thoughts?

runeh commented 7 years ago

I ended up changing our code to deal the promise rejection in the renderer. But that seems unidiomatic, given that there are tags meant for dealing with timeouts and errors inside of await?

Anyway, it still feels counter-intuitive to me that there is console.error output in a case where I've explicitly added code to deal with the error. I would prefer it not logging on my behalf in that case.

mlrawlings commented 6 years ago

Perhaps we should emit the error (out.emit('await:error', err)) so that it could be picked up by a logging tool, but would be opt-in.

SimenB commented 6 years ago

AsyncStream has the same issue: https://github.com/marko-js/marko/blob/bcecb400778ec30fcc88075bd8276b4c83236d9c/src/runtime/html/AsyncStream.js#L441-L443

DylanPiercey commented 4 years ago

This is no longer the case.