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

URL replacements should not log, but rather report to the user if replacement promise failed #2418

Closed
cramforce opened this issue Mar 3, 2016 · 3 comments
Closed

URL replacements should not log, but rather report to the user if replacement promise failed #2418

cramforce opened this issue Mar 3, 2016 · 3 comments
Assignees
@cramforce
Labels
P1: High Priority Type: Bug
Milestone
Fixit - March 2016

Comments

@cramforce
Copy link
Member

Currently those errors get eaten.

See

amphtml/src/url-replacements.js

Line 389 in 2dd024d

log.error(TAG_, 'Failed to expand: ' + name, err);

@cramforce cramforce mentioned this issue Mar 3, 2016
Closed
@jridgewell
Copy link
Contributor

What are you asking me to do? Just rethrow the error?

@cramforce
Copy link
Member Author

Yeah, I think we want to maintain the property that we resolve the promise, but throw in a timeout.

@rudygalfi rudygalfi modified the milestone: Fixit - March 2016 Mar 3, 2016
@cramforce cramforce assigned cramforce and unassigned jridgewell Mar 9, 2016
@cramforce
Copy link
Member Author

Taking this since Justin is out.

cramforce added a commit to cramforce/amphtml that referenced this issue Mar 9, 2016
@cramforce
Don't swallow errors in URL replacements promises.
ea25e6d 
And unifies the behavior across sync and non-sync replacements.
Both now replace with the empty string and report the error to the console and the server where applicable. It would have been an option to fail the URL generation instead (previous that was the behavior for sync errors, but I believe that erring on the side of producing a URL is the right thing to do.

Fixes ampproject#2418
@cramforce cramforce mentioned this issue Mar 9, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cramforce cramforce

Labels
P1: High Priority Type: Bug
Projects
None yet
Milestone
Fixit - March 2016
Development

No branches or pull requests
3 participants
@cramforce @jridgewell @rudygalfi