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

clean up of extensible audit loading and error handling #679

Merged
merged 2 commits into from
Sep 21, 2016

Conversation

brendankenny
Copy link
Member

@brendankenny brendankenny commented Sep 21, 2016

This came from @ebidel making his own custom audits. Any errors in a custom audit (e.g. syntax error or transitive dependencies not being found) were being absorbed and reemitted as "Unable to locate audit", which is not true and confusing.

In fixing this, however, it also became clear that our audit resolution was being done in a really fragile way, so this PR changes a few things:

  • Only assume we can use a core audit path to load an audit if the audit is in fact a core audit. Using this path as the default value makes for confusing error messages if the audit is never found.
  • Next, attempt to require() in place. No one is going to specify their paths relative to lighthouse-core/config/config.js, but this handles the audit-in-an-npm module case by walking up and down the path looking for audits in node_modules/ directories.
  • Next, look for the audit relative to process.cwd(). Resolving relative to the working directory is mostly to handle the case of using Lighthouse as a module since then you're passing in a config object, not a file, so there is no config file path.
  • Next, as before, look for the audit relative to the config file path, if there is one. We need to make sure the config path is an absolute one by this point, however, as it's unlikely for the config file to be specified relative to config.js, and we won't always be able to locate it ourselves by this point. I've put in an assertion in the Config constructor to make sure the path is absolute by that point and made sure the CLI handles that properly.
  • Finally, only emit an 'Unable to locate audit' error in the case of a not found audit. For all other errors, let the normal error flow handle it.

@brendankenny
Copy link
Member Author

gather loading will have to get the same treatment. I'll do that in a followup PR.

@paulirish
Copy link
Member

Nice.

Can you add some inline docs to the source with the summary you provided here?

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

lgtm % inline docs

Copy link
Member

@addyosmani addyosmani left a comment

Choose a reason for hiding this comment

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

Code changes lgtm. Docs comment above also sg.

@brendankenny
Copy link
Member Author

brendankenny commented Sep 21, 2016

realized I had accidentally taken out support for using an audit from an npm module, so added that back. Moved to a new function so it can be flat instead of three nested try/catches. Added documentation discussed above.

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.

3 participants