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.

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

Amp-font: Un-handled Promise Rejection #1095 #1107

Merged
merged 1 commit into from
Dec 9, 2015

Conversation

camelburrito
Copy link
Contributor

Amp-font: Unhandled Promise Rejection #1095

@@ -124,7 +128,7 @@ export class AmpFont extends AMP.BaseElement {
this.onFontLoadSuccess_();
}).catch(error => {
this.onFontLoadError_();
throw error;
log.warn(TAG_, error, this.element);
Copy link
Member

Choose a reason for hiding this comment

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

The logger isn't the right tool in its current form as content devs never see it.

@erwinmombay was looking into fixing that, but I believe for now we should use console.warn/log.

Copy link
Member

Choose a reason for hiding this comment

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

@cramforce lets go with that, that'll make it easier for me to separate them.

Copy link
Member

Choose a reason for hiding this comment

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

@erwinmombay Go with what?

Copy link
Member

Choose a reason for hiding this comment

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

@cramforce using console.log/warn, i have a branch that separates amp-dev logs vs publisher-dev logs, so this makes it easier for me to follow up. (where console.log/warn will always be publisher-dev logs)

Copy link
Member

Choose a reason for hiding this comment

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

SGTM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dvoytenko suggested log.warn - as opposed to console.warn.

I have added a warn. @cramforce Comment on which one needs removal now

Ideally it would be nice to have log.warn decide which ones should be console.warn'd, rather than using 2 different logging systems

Copy link
Member

Choose a reason for hiding this comment

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

@sriramkrish85 @erwinmombay is working on unifying the 2.

For now add console.warn

@erwinmombay
Copy link
Member

@sriramkrish85 LGTM

@erwinmombay erwinmombay assigned erwinmombay and unassigned dvoytenko Dec 9, 2015
camelburrito pushed a commit that referenced this pull request Dec 9, 2015
Amp-font: Un-handled Promise Rejection #1095
@camelburrito camelburrito merged commit 5479135 into ampproject:master Dec 9, 2015
@camelburrito camelburrito deleted the fontbug branch December 9, 2015 03:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants