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

Suppress Console Messages in AMP output #609

Merged
merged 8 commits into from
Aug 14, 2019
Merged

Conversation

kristoferbaxter
Copy link
Contributor

@kristoferbaxter kristoferbaxter commented Aug 14, 2019

Each distributed thread's code for amp-script integration includes additional logging information.

This PR removes the logging since it can impact performance.

This PR uses the similar to AMP logic for retaining logs (presence of development or log in location.hash.

Edit: Changed approach based on PR feedback.

Copy link
Collaborator

@dreamofabear dreamofabear left a comment

Choose a reason for hiding this comment

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

We should probably delegate logging instead so we can be consistent with AMP's logging system.

@kristoferbaxter kristoferbaxter merged commit 97869fe into master Aug 14, 2019
@kristoferbaxter kristoferbaxter deleted the suppress_output_amp branch August 14, 2019 20:17
@westonruter
Copy link
Member

I thought that Renovate would have automatically opened a PR to update worker-dom in amphtml but it's actually two versions behind now:

https://github.com/ampproject/amphtml/blob/b5494ca9c9496e0ff31a1d47f09e0805c3473aa0/package.json#L22

@estherkim
Copy link
Collaborator

Hey @westonruter, we purposely disable renovate-bot for dependencies. https://github.com/ampproject/amphtml/blob/master/renovate.json#L13

@dreamofabear
Copy link
Collaborator

I'll upgrade amphtml soon.

@rsimha
Copy link

rsimha commented Aug 16, 2019

I thought that Renovate would have automatically opened a PR...

This was true for both dependencies and devDependencies in the past, but at some point, it was decided to restrict automatic updates to just devDependencies. See ampproject/amphtml#17394 (comment) and ampproject/amphtml#23008.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants