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

new_audit: add js-libraries audit, just listing detected js libs #6081

Merged
merged 4 commits into from Sep 22, 2018
Merged

Conversation

@paulirish
Copy link
Member

@paulirish paulirish commented Sep 20, 2018

cc @housseindjirdeh

output in the LHR looks like this:

{
  "id": "js-libraries",
  "title": "JavaScript libraries",
  "description": "All detected front-end JavaScript libraries on the page",
  "score": 1,
  "scoreDisplayMode": "binary",
  "rawValue": true,
  "details": {
    "type": "table",
    "headings": [
      {
        "key": "name",
        "itemType": "text",
        "text": "Name"
      },
      {
        "key": "version",
        "itemType": "text",
        "text": "Version"
      }
    ],
    "items": [
      {
        "name": "jQuery",
        "version": "1.12.4",
        "npm": "jquery"
      },
      {
        "name": "jQuery UI",
        "version": "1.11.4",
        "npm": "jquery-ui"
      },
      {
        "name": "Modernizr",
        "version": "2.6.2",
        "npm": "modernizr"
      },
      {
        "name": "Underscore",
        "version": "1.8.3",
        "npm": "underscore"
      },
      {
        "name": "FlexSlider",
        "version": null,
        "npm": "flexslider"
      }
    ],
    "summary": {}
  }
}
@paulirish
Copy link
Member Author

@paulirish paulirish commented Sep 20, 2018

cc @rviscomi to review the data format

@developit
Copy link
Member

@developit developit commented Sep 20, 2018

Looks good! This should be relatively easy to parse from within BQ.

@rviscomi - any thoughts about whether this format makes it possible to flatten the list of frameworks down so it's more readily accessible (as columns)?

@patrickhulce
Copy link
Collaborator

@patrickhulce patrickhulce commented Sep 20, 2018

we should also probably bump our version of the detector, last time I checked they were missing ~90% of react sites I tested and a few commits helped lower that significantly

@wardpeet
Copy link
Member

@wardpeet wardpeet commented Sep 20, 2018

this is awesome :)

@rviscomi
Copy link
Member

@rviscomi rviscomi commented Sep 20, 2018

any thoughts about whether this format makes it possible to flatten the list of frameworks down so it's more readily accessible (as columns)?

Yeah with a user-generated function we're able to massage this formatting however we need. This format LGTM.

@paulirish
Copy link
Member Author

@paulirish paulirish commented Sep 20, 2018

we should also probably bump our version of the detector, last time I checked they were missing ~90% of react sites I tested and a few commits helped lower that significantly

yup. already on it: johnmichel/Library-Detector-for-Chrome#118

Copy link
Member

@brendankenny brendankenny left a comment

LGTM! 📚📚📚📚📚📚

@@ -0,0 +1,52 @@
/**
* @license Copyright 2017 Google Inc. All Rights Reserved.

This comment has been minimized.

@@ -0,0 +1,64 @@
/**
* @license Copyright 2017 Google Inc. All Rights Reserved.

This comment has been minimized.

});
assert.equal(auditResult1.rawValue, true);

// duplicates. TODO: consider failing in this case

This comment has been minimized.

@brendankenny

brendankenny Sep 20, 2018
Member

TODO: consider failing in this case

it's not that uncommon to have more than one jquery in one page. And if we merge network stuff from iframes, won't it be even more common?

@@ -87,7 +87,7 @@ declare global {

export type DetailsItem = string | number | DetailsRendererNodeDetailsJSON |
DetailsRendererLinkDetailsJSON | DetailsRendererCodeDetailJSON | undefined |
boolean | DetailsRendererUrlDetailsJSON;
boolean | DetailsRendererUrlDetailsJSON | null;

This comment has been minimized.

@brendankenny

brendankenny Sep 20, 2018
Member

details make me sad

Copy link
Member

@exterkamp exterkamp left a comment

LGTM, but does this new Audit need i18n on it's strings?

@exterkamp
Copy link
Member

@exterkamp exterkamp commented Sep 21, 2018

❌ FAIL. LHR has changed. Run 'yarn update:sample-json' to rebaseline the golden LHR. error Command failed with exit code 1. info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command. The command "yarn diff:sample-json" exited with 1.

Need to update the sample-json with the new audit table?

@paulirish paulirish merged commit 417cdbe into master Sep 22, 2018
7 checks passed
7 checks passed
bundlesize Good job! bundle size < maxSize
Details
@googlebot
cla/google All necessary CLAs are signed
@codecov
codecov/patch 100% of diff hit (target 91.85%)
Details
@codecov
codecov/project 91.86% (+0.01%) compared to bb8e030
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
pr title lint PR title is good to go, boss
Details
@paulirish paulirish deleted the usedlibs branch Sep 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants