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.

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

Option: disable log output for riot-tmpl #2108

Closed
fabien opened this Issue Nov 27, 2016 · 14 comments

Comments

Projects
None yet
5 participants
@fabien
Contributor

fabien commented Nov 27, 2016

  1. Describe your issue:

3.0.0 introduced this breaking change:

Change: template errors will be always output via console.error if the console api is available (breaking change)

While I applaud having this output while developing, I would like an option/global flag to disable this explicitly i.e. in production.

PS is it bad practice to simply do { error.message } instead of the long-winded (but more correct) { error && error.message } in templates? Until these errors were output, I figured it was a nicety of Riot to not trip over this.

  1. Can you reproduce the issue?

It will trip over a lot of cases that previously were silently handled.

  1. On which browser/OS does the issue appear?

Any

  1. Which version of Riot does it affect?

3.0.x

  1. How would you tag this issue?
  • Question
  • Bug
  • Discussion
  • Feature request
  • Tip
  • Enhancement
  • Performance
@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Nov 27, 2016

Member

You can simply use:

console.error = function() { /* noop */  }
Member

GianlucaGuarini commented Nov 27, 2016

You can simply use:

console.error = function() { /* noop */  }
@fabien

This comment has been minimized.

Show comment
Hide comment
@fabien

fabien Nov 27, 2016

Contributor

Yes, sure, but I'd prefer something like this instead:

riot.logError = function() { /* noop */  }

Similar, but less intrusive, don't you think?

Contributor

fabien commented Nov 27, 2016

Yes, sure, but I'd prefer something like this instead:

riot.logError = function() { /* noop */  }

Similar, but less intrusive, don't you think?

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Nov 27, 2016

Member

I am not sure, I mean in the end an error is an error! Riot will use the console.error api so basically in production it shouldn't have any side effect, it does not throw new Error(). I think it's a fair decision instead of allowing our users to swallow the template errors via option.
I would like to hear the opinion of the other @riot/core-maintainers on this

Member

GianlucaGuarini commented Nov 27, 2016

I am not sure, I mean in the end an error is an error! Riot will use the console.error api so basically in production it shouldn't have any side effect, it does not throw new Error(). I think it's a fair decision instead of allowing our users to swallow the template errors via option.
I would like to hear the opinion of the other @riot/core-maintainers on this

@fabien

This comment has been minimized.

Show comment
Hide comment
@fabien

fabien Nov 28, 2016

Contributor

@GianlucaGuarini what's your take on the practice of using { foo.bar.baz } directly, as opposed to { foo && foo.bar && foo.bar.baz }? This is my main concern actually, I would hate to fall back to the 'correct' but tedious syntax like that.

Contributor

fabien commented Nov 28, 2016

@GianlucaGuarini what's your take on the practice of using { foo.bar.baz } directly, as opposed to { foo && foo.bar && foo.bar.baz }? This is my main concern actually, I would hate to fall back to the 'correct' but tedious syntax like that.

@cognitom

This comment has been minimized.

Show comment
Hide comment
@cognitom

cognitom Nov 28, 2016

Member

@GianlucaGuarini I have no need to make it optional, at this point. Less is better.
@fabien { foo.bar.baz } question seems another topic. Could you open a new issue?

Member

cognitom commented Nov 28, 2016

@GianlucaGuarini I have no need to make it optional, at this point. Less is better.
@fabien { foo.bar.baz } question seems another topic. Could you open a new issue?

@fabien fabien referenced this issue Nov 28, 2016

Closed

Use of dot-notation properties in expressions #2114

1 of 7 tasks complete
@cognitom

This comment has been minimized.

Show comment
Hide comment
@cognitom

cognitom Nov 29, 2016

Member

Oh, I had forgot that there's riot.util.tmpl.errorHandler option.
http://riotjs.com/api/misc/#a-nametmpl-errorsa-riotutiltmplerrorhandler

@GianlucaGuarini @aMarCruz how about suppressing default error messages if users define their own handlers? This seems reasonable for me.

We could easily do it by putting the code into else block:
https://github.com/riot/tmpl/blob/v3.0.0/src/tmpl.js#L126-L134

Member

cognitom commented Nov 29, 2016

Oh, I had forgot that there's riot.util.tmpl.errorHandler option.
http://riotjs.com/api/misc/#a-nametmpl-errorsa-riotutiltmplerrorhandler

@GianlucaGuarini @aMarCruz how about suppressing default error messages if users define their own handlers? This seems reasonable for me.

We could easily do it by putting the code into else block:
https://github.com/riot/tmpl/blob/v3.0.0/src/tmpl.js#L126-L134

@fabien

This comment has been minimized.

Show comment
Hide comment
@fabien

fabien Nov 29, 2016

Contributor

@cognitom this is exactly what I had in mind

Contributor

fabien commented Nov 29, 2016

@cognitom this is exactly what I had in mind

@aMarCruz

This comment has been minimized.

Show comment
Hide comment
@aMarCruz

aMarCruz Nov 30, 2016

Member

The console.error addition is not mine. The porpouse of tmpl.errorHandler was let the choice to the user.

Member

aMarCruz commented Nov 30, 2016

The console.error addition is not mine. The porpouse of tmpl.errorHandler was let the choice to the user.

@cognitom

This comment has been minimized.

Show comment
Hide comment
@cognitom

cognitom Nov 30, 2016

Member

@aMarCruz I c. I'll try to send PR about this. Pls check that later 😄

Member

cognitom commented Nov 30, 2016

@aMarCruz I c. I'll try to send PR about this. Pls check that later 😄

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Nov 30, 2016

Member

@aMarCruz @cognitom swallowing template errors it's a really bad idea unless you really know what you are doing. Considering that you could easily handle it outside of riot http://plnkr.co/edit/7D7QKAigH6GFHEATkOpk?p=preview and that 95% of userland don't know what they are doing I guess it's a good idea to keep it strict. However I guess that @cognitom solution could be an acceptable compromise
✌️

Member

GianlucaGuarini commented Nov 30, 2016

@aMarCruz @cognitom swallowing template errors it's a really bad idea unless you really know what you are doing. Considering that you could easily handle it outside of riot http://plnkr.co/edit/7D7QKAigH6GFHEATkOpk?p=preview and that 95% of userland don't know what they are doing I guess it's a good idea to keep it strict. However I guess that @cognitom solution could be an acceptable compromise
✌️

@fabien

This comment has been minimized.

Show comment
Hide comment
@fabien

fabien Nov 30, 2016

Contributor

While I agree with @GianlucaGuarini in principle, I find that in practice, I know exactly what I'm doing. Therefor, I would like to be in charge of how to treat errors as they arise.

@cognitom looking forward to your PR

Contributor

fabien commented Nov 30, 2016

While I agree with @GianlucaGuarini in principle, I find that in practice, I know exactly what I'm doing. Therefor, I would like to be in charge of how to treat errors as they arise.

@cognitom looking forward to your PR

@cognitom

This comment has been minimized.

Show comment
Hide comment
@cognitom

cognitom Nov 30, 2016

Member

Done. I've just inserted else, haha.

Member

cognitom commented Nov 30, 2016

Done. I've just inserted else, haha.

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Feb 4, 2017

Member

this patch solved the issues riot/tmpl#19

Member

GianlucaGuarini commented Feb 4, 2017

this patch solved the issues riot/tmpl#19

@sourcegr

This comment has been minimized.

Show comment
Hide comment
@sourcegr

sourcegr Feb 4, 2017

now be prepared for the "dont break the versioning" thing again :P

sourcegr commented Feb 4, 2017

now be prepared for the "dont break the versioning" thing again :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment