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

Prevent or intercept AMP Optimizer warning in console #1263

Open
andrewmanullang opened this issue Sep 15, 2021 · 12 comments
Open

Prevent or intercept AMP Optimizer warning in console #1263

andrewmanullang opened this issue Sep 15, 2021 · 12 comments

Comments

@andrewmanullang
Copy link

Hi, is there any way to prevent or intercept AMP Optimizer warning on console?

AMP Optimizer WARNING Found 8 hero elements on the page, but the maximum is set to 4. The limit can be configured via the 'maxHeroImage' parameter.

Thanks

@samouri
Copy link
Member

samouri commented Sep 20, 2021

+1, I've ran into this before as well. Given how expected this message is, we should a way of disabling it. Some ideas:

  • (best) Only emit in verbose mode
  • Remove the message entirely
  • Create a quiet mode if we don't already have one

@westonruter
Copy link
Member

The OptimizeHeroImage transformer has a maxHeroImageCount parameter, but it defaults to 2 not 4. Maybe you have a custom config that sets it to 4 but you could just increase it to 8? Or maybe you are already doing this and the warning is expected, but just not desired.

@patrickkettner
Copy link
Collaborator

patrickkettner commented Sep 20, 2021

few things

  • On this specific issue, you can make it go away by raising maxHeroImage value - are you doing this currently?
  • You can supply the log attribute in your config (this is the default). You can add custom logic or to that if you wanted to keep some logging, or just supply an empty object with the same signature to /dev/null everything

@samouri
Copy link
Member

samouri commented Sep 20, 2021

Please let me know if I'm misunderstanding:

Let's say I'm building a page with 10 images and I know for certain only the top 4 would ever be in the first viewport.
Isn't it reasonable for me to set maxHeroImageCount to 4? In that case, the warning is just noise.

@andrewmanullang
Copy link
Author

+1, I've ran into this before as well. Given how expected this message is, we should a way of disabling it. Some ideas:

  • (best) Only emit in verbose mode
  • Remove the message entirely
  • Create a quiet mode if we don't already have one

Agree. Since the errors come from AMP Optimizer is causing unnecessary logs in production, which is very costly.
My only solution now is by hijacking the console.warning.

if (production) {
  console.warn = function() {}
}

@andrewmanullang
Copy link
Author

The OptimizeHeroImage transformer has a maxHeroImageCount parameter, but it defaults to 2 not 4. Maybe you have a custom config that sets it to 4 but you could just increase it to 8? Or maybe you are already doing this and the warning is expected, but just not desired.

Just to be clear, regarding the warning, yes the fix is to increase the maxHeroImage, but my issue is more about the warning that come from AMP optimizer, because it can be repetitive and increase the logs

@andrewmanullang
Copy link
Author

Please let me know if I'm misunderstanding:

Let's say I'm building a page with 10 images and I know for certain only the top 4 would ever be in the first viewport.
Isn't it reasonable for me to set maxHeroImageCount to 4? In that case, the warning is just noise.

yes, I don't think thats an issue and wouldn't create any warning.

@andrewmanullang
Copy link
Author

few things

  • On this specific issue, you can make it go away by raising maxHeroImage value - are you doing this currently?
  • You can supply the log attribute in your config (this is the default). You can add custom logic or to that if you wanted to keep some logging, or just supply an empty object with the same signature to /dev/null everything
  • yes, I fixed the maxHeroImage
  • can you show me how to use the config with the log attribute, or is it already documented somewhere? basically just to prevent the warning if it is in production mode

@patrickkettner
Copy link
Collaborator

can you show me how to use the config with the log attribute, or is it already documented somewhere? basically just to prevent the warning if it is in production mode

Wherever you are providing your config , e.g.

const ampOptimizer = AmpOptimizer.create({
  extensionVersions: {
    'amp-twitter': '0.1',
  },
});

you can add a log attribute to override it

const ampOptimizer = AmpOptimizer.create({
  extensionVersions: {
    'amp-twitter': '0.1',
  },
  log: {
    info: () => {},
    warn: () => {},
    debug: () => {},
    error: () => {},
  }
});

You could do this more cleanly by building off of the existing one

const myLog = require('@ampproject/toolbox-core/lib/Log.js');
myLog.output_.originalWarn = myLog.output_.warn
myLog.output_.warn = (() => { process.env.I_AM_IN_PRODUCTION_AND_KNOW_THIS_BECUASE_OF_AN_ENV_VAR ? () => {} : myLog.output_.originalWarn})()


const ampOptimizer = AmpOptimizer.create({
  extensionVersions: {
    'amp-twitter': '0.1',
  },
  log: myLog
});


does that make sense?

@andrewmanullang
Copy link
Author

Wow nice, I will try this. Thanks @patrickkettner for the detailed solution.

@patrickkettner
Copy link
Collaborator

@sebastianbenz would it make sense to create a logLevel attribute or something more built-in for this sort of thing?

@sebastianbenz
Copy link
Collaborator

Thanks for the workaround @patrickkettner (I just wanted to write the same)! Loglevels would be the best solution, but in this case it'd probably make more sense to print this message only in debug mode.

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

No branches or pull requests

5 participants