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

core(stack-packs): add angular, react, amp, and magento packs #9797

Merged

Conversation

housseindjirdeh
Copy link
Contributor

@housseindjirdeh housseindjirdeh commented Oct 7, 2019

Summary:

This PR brings in some fresh 🍅new stack packs:

  • Angular
  • React
  • AMP
  • Magento

@brendankenny
Copy link
Member

🎉 🎉

Copy link
Member

@exterkamp exterkamp left a comment

Choose a reason for hiding this comment

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

Do you have some test sites that we can use to see what this looks like/I can make sure it's all i18n'd 😄

stack-packs/packs/angular.js Outdated Show resolved Hide resolved
stack-packs/packs/react.js Show resolved Hide resolved
@housseindjirdeh
Copy link
Contributor Author

housseindjirdeh commented Oct 8, 2019

@exterkamp some sites that can be tested on:

Let me know if you need any more URLs and will send them your way 🚀

@housseindjirdeh
Copy link
Contributor Author

Note that Magento detection is still not in in the library detector extension.

johnmichel/Library-Detector-for-Chrome#142

I just put up a PR and will get it merged in today soon as I get it reviewed by someone from their team 👍

Copy link
Member

@exterkamp exterkamp left a comment

Choose a reason for hiding this comment

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

So I don't think we ever wrote down the process for adding a stack pack.

  1. Import the js ✔️
  2. Add the js to the stack-packs/index.js requires block ❌
/** @type {typeof import('./stack-packs')} */
const stackPacks = [
  require('./packs/wordpress.js'),
  require('./packs/react.js'),
  require('./packs/angular.js'),
  require('./packs/amp.js'),
  require('./packs/magento.js'),
];
  1. Add the stack pack id's to the lighthouse-core/lib/stack-packs.js enabled packs ❌
// TODO(Issue #): import magento once detector has landed, and it has an id.
const stackPacksToInclude = [{
  packId: 'wordpress',
  requiredStacks: ['js:wordpress'],
}, {
  packId: 'react',
  requiredStacks: ['js:react'],
}, {
  packId: 'angular',
  requiredStacks: ['js:@angular/core'],
}, {
  packId: 'amp',
  requiredStacks: ['js:amp'],
}];

Then it should be g2g for the report:
image
image
image
image

They look great @housseindjirdeh!

Also if you want to add a README.md to /stack-packs outlining this process I think it needs one. Something like...

# Uploading a new stack pack

1. Add stack pack .js file

2. Add file to the require() in `index.js`

3. Add new stack pack to included packs in `stack-packs.js`

## FAQs

* Parsing error on your SVG? Make sure you escape `#` into `%23`.

stack-packs/packs/amp.js Outdated Show resolved Hide resolved
stack-packs/packs/angular.js Outdated Show resolved Hide resolved
stack-packs/packs/magento.js Outdated Show resolved Hide resolved
stack-packs/packs/react.js Outdated Show resolved Hide resolved
Copy link
Member

@exterkamp exterkamp left a comment

Choose a reason for hiding this comment

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

Code LGTM. nits about the descriptions and strings now 💬


1. Add a new stack pack file to the `packs/` directory. This file needs to include a few exported properties:

```
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
```
```javascript

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

```
module.exports = {
id: ..., // ID
iconDataURL: ..., //SVG encoded icon
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
iconDataURL: ..., //SVG encoded icon
iconDataURL: ..., // SVG encoded icon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed


2. Import your new stack pack in `index.js` in this directory:

```
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
```
```javascript

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated


3. Add new stack pack to included packs in `lighthouse-core/lib/stack-packs.js`:

```
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
```
```javascript

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

const ampIcon = `data:image/svg+xml,%3Csvg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 256 256"%3E%3Cpath d="M171.887 116.28l-53.696 89.36h-9.728l9.617-58.227-30.2.047a4.852 4.852 0 01-4.855-4.855c0-1.152 1.07-3.102 1.07-3.102l53.52-89.254 9.9.043-9.86 58.317 30.413-.043a4.852 4.852 0 014.855 4.855c0 1.088-.427 2.044-1.033 2.854l.004.004zM128 0C57.306 0 0 57.3 0 128s57.306 128 128 128 128-57.306 128-128S198.7 0 128 0z" fill="%230379c4" fill-rule="evenodd"/%3E%3C/svg%3E`;

const UIStrings = {
/** Additional description of a Lighthouse audit that tells the user how they can improve image loading by using webp in the context of AMP. This is displayed after a user expands the section to see more. No character length limits. Links in (parenthesis) become link texts to additional documentation. */
Copy link
Member

Choose a reason for hiding this comment

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

nit: match the style of the description

Suggested change
/** Additional description of a Lighthouse audit that tells the user how they can improve image loading by using webp in the context of AMP. This is displayed after a user expands the section to see more. No character length limits. Links in (parenthesis) become link texts to additional documentation. */
/** Additional description of a Lighthouse audit that tells the user how they can improve image loading by using WebP in the context of AMP. This is displayed after a user expands the section to see more. No character length limits. Links in (parenthesis) become link texts to additional documentation. */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 🌮

/** Additional description of a Lighthouse audit that tells the user how they can improve site loading performance by reducing the total bytes delivered by their page in the context of the Magento platform. This is displayed after a user expands the section to see more. No character length limits. Links in (parenthesis) become link texts to additional documentation. */
total_byte_weight: 'Disable Magento\'s built-in [JavaScript bundling and minification](https://devdocs.magento.com/guides/v2.3/frontend-dev-guide/themes/js-bundling.html), and consider using [baler](https://github.com/magento/baler/).',
/** Additional description of a Lighthouse audit that tells the user how they can improve performance by reducing the amount of render blocking resources present on their page, in the context of the Magento platform. This is displayed after a user expands the section to see more. No character length limits. Links in (parenthesis) become link texts to additional documentation. */
render_blocking_resources: 'Disable Magento\'s built-in [JavaScript bundling and minification](https://devdocs.magento.com/guides/v2.3/frontend-dev-guide/themes/js-bundling.html), and consider using [baler](https://github.com/magento/baler/) instead.',
Copy link
Member

Choose a reason for hiding this comment

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

Is this functionally the same as total_byte_weight? Should they share a string and then remove one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep they're the same, updated!

/** Additional description of a Lighthouse audit that tells the user how they can improve performance by reducing the amount of render blocking resources present on their page, in the context of the Magento platform. This is displayed after a user expands the section to see more. No character length limits. Links in (parenthesis) become link texts to additional documentation. */
render_blocking_resources: 'Disable Magento\'s built-in [JavaScript bundling and minification](https://devdocs.magento.com/guides/v2.3/frontend-dev-guide/themes/js-bundling.html), and consider using [baler](https://github.com/magento/baler/) instead.',
/** Additional description of a Lighthouse audit that tells the user how they can improve performance by minifying their CSS files in the context of the Magento platform. This is displayed after a user expands the section to see more. No character length limits. Links in (parenthesis) become link texts to additional documentation. */
unminified_css: 'Enable the "Minify CSS Files" option in the store\'s Developer settings.',
Copy link
Member

Choose a reason for hiding this comment

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

nit: "the store's" can be ambiguous, is it the Magento store? Or my site's store?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to make it more clear (and included a link as well)

/** Additional description of a Lighthouse audit that tells the user how they can improve performance by minifying their Javascript files in the context of the React library. This is displayed after a user expands the section to see more. No character length limits. Links in (parenthesis) become link texts to additional documentation. */
unminified_javascript: 'If your build system minifies your JS files automatically, ensure that you are deploying the production build of your application. You can check this with the React Developer Tools extension. [Learn more](https://reactjs.org/docs/optimizing-performance.html#use-the-production-build).',
/** Additional description of a Lighthouse audit that tells the user how they can improve performance by removing unused Javascript files in the context of the React library. This is displayed after a user expands the section to see more. No character length limits. Links in (parenthesis) become link texts to additional documentation. */
unused_javascript: 'If you are not server-side rendering, split your JavaScript bundles with [React.lazy()](https://web.dev/code-splitting-suspense/). Otherwise, code-split using a third-party library such as [loadable-components](https://www.smooth-code.com/open-source/loadable-components/docs/getting-started/).',
Copy link
Member

Choose a reason for hiding this comment

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

note: this is good evidence for code-spans in links.

nit: React.lazy() could be in code spans, and put the link somewhere else?

Suggested change
unused_javascript: 'If you are not server-side rendering, split your JavaScript bundles with [React.lazy()](https://web.dev/code-splitting-suspense/). Otherwise, code-split using a third-party library such as [loadable-components](https://www.smooth-code.com/open-source/loadable-components/docs/getting-started/).',
unused_javascript: 'If you are not server-side rendering, [split your JavaScript bundles](https://web.dev/code-splitting-suspense/) with `React.lazy()`. Otherwise, code-split using a third-party library such as [loadable-components](https://www.smooth-code.com/open-source/loadable-components/docs/getting-started/).',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated 👍

/** Additional description of a Lighthouse audit that tells the user how they can improve the time to first byte speed metric, in the context of the React library. This is displayed after a user expands the section to see more. No character length limits. Links in (parenthesis) become link texts to additional documentation. */
time_to_first_byte: 'If you are server-side rendering any React components, consider using `renderToNodeStream()` or `renderToStaticNodeStream()` to allow the client to receive and hydrate different parts of the markup instead of all at once. [Learn more](https://reactjs.org/docs/react-dom-server.html#rendertonodestream).',
/** Additional description of a Lighthouse audit that tells the user how they can minimize redirects, in the context of the React library. This is displayed after a user expands the section to see more. No character length limits. Links in (parenthesis) become link texts to additional documentation. */
redirects: 'If you are using React Router, minimize usage of the [<Redirect>](https://reacttraining.com/react-router/web/api/Redirect) component.',
Copy link
Member

Choose a reason for hiding this comment

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

nit: If <Redirect> is an html node, then put it in a code span and move the link.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

/** Additional description of a Lighthouse audit that tells the user how they can use the Profiler to help measure performance. This is displayed after a user expands the section to see more. No character length limits. Links in (parenthesis) become link texts to additional documentation. */
user_timings: 'Use the React DevTools Profiler, which makes use of the Profiler API, to measure the rendering performance of your components. [Learn more.](https://reactjs.org/blog/2018/09/10/introducing-the-react-profiler.html)',
/** Additional description of a Lighthouse audit that tells the user *why* and *how* they should reduce the size of the web page's DOM, in the context of the React library, as well as how to maximize component performance when many DOM nodes are present. Links in (parenthesis) become link texts to additional documentation. */
dom_size: 'If you are rendering very large lists, consider using a “windowing” library like `react-window` to minimize the number of DOM nodes created. [Learn more](https://web.dev/virtualize-long-lists-react-window/). Also, minimize unecessary re-renders using [shouldComponentUpdate](https://reactjs.org/docs/optimizing-performance.html#shouldcomponentupdate-in-action), [PureComponent](https://reactjs.org/docs/react-api.html#reactpurecomponent), or [React.memo](https://reactjs.org/docs/react-api.html#reactmemo) and [skip effects](https://reactjs.org/docs/hooks-effect.html#tip-optimizing-performance-by-skipping-effects) only until certain dependencies have changed if you are using the Effect hook to improve runtime performance.',
Copy link
Member

Choose a reason for hiding this comment

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

This seems like specific advice for lists specifically, not for any large DOM size?

I'm not a react dev, so I might be off base, but the first part seems too specific.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can vouch for a very common reason DOMs end up so large is because of repeated elements that aren't using occlusion culling. Might want to tweak to be less focused on lists as opposed to all repeated elements (tables, lists, grids, anything that scrolls), but personally I think this is some pretty good specific advice.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah +1 it seems like good advice, but it just seems odd to have specific advice like that first.

Copy link
Contributor Author

@housseindjirdeh housseindjirdeh Oct 10, 2019

Choose a reason for hiding this comment

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

Hmm that's a great point. Mentioning "lists" specifically is probably not the best.

Updated to: Consider using a “windowing” library like react-window to minimize the number of DOM nodes created if you are rendering many **repeated elements** on the page...

Copy link
Member

@exterkamp exterkamp left a comment

Choose a reason for hiding this comment

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

LGTM, just needs to update:sample-json and get all the tests in order!

@housseindjirdeh housseindjirdeh changed the title Fresh new stack packs core(stack-packs): fresh new stack packs Oct 14, 2019
@housseindjirdeh housseindjirdeh changed the title core(stack-packs): fresh new stack packs core(stack-packs): fresh new packs Oct 14, 2019
@housseindjirdeh
Copy link
Contributor Author

@exterkamp done, and tests are passing

Note: should I be adding these packs and false detections to dbw?

@exterkamp
Copy link
Member

Hm, that might be a question more suited fro @brendankenny. My instinct is yes, add them to the new expectations to get that 💯 , but there might be a more nuanced view.

@brendankenny
Copy link
Member

Note: should I be adding these packs and false detections to dbw?

The biggest benefits of getting a stack pack into the dbw test in #8536 were

  • the end to end test (stack packs detector runs, result ended up in the lhr) and
  • a stack pack in sample_v2 and the now deployment so we could see how they look live after every PR (ensuring the lhr result ended up in the html report and it rendered ok)

Now that we'll have a bunch of new stack packs, adding them all to dbw would really only add

  • can detect and display more than one stack pack at once
  • each of these stacks are indeed detected

dbw is probably complicated enough that it doesn't need new stacks added just to pull those off, so how about we make a new smoke test(s) for these stacks? Better to start fresh.

I'm not sure how far down this path we want to go (since at some point it'll become more a test of JS LIbrary detector than of Lighthouse), but a basic multi-stack test doesn't seem like a ton of overhead.

Should probably be in a follow up PR, though, let's land this one :)

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

the other smoke test failure comes from what looks like the update to js-library-detector now also having a "jQuery (Fast path)" test, so I think you might need

Stacks: [{
  id: 'jquery',
}, {
  id: 'jquery',
}, {
  id: 'wordpress',
}],

in the expectations now? Maybe drop a comment or something on the second one

yarn.lock Outdated
@@ -5,14 +5,12 @@
"@babel/code-frame@^7.0.0":
version "7.0.0"
resolved "https://registry.yarnpkg.com/@babel/code-frame/-/code-frame-7.0.0.tgz#06e2ab19bdb535385559aabb5ba59729482800f8"
integrity sha512-OfC2uemaknXr87bdLUkWog7nYuliM9Ij5HUcajsVcMCpQrcLmtxRbVFTIqmcSkSeYRBFBRxs2FiUqFJDLdiebA==
Copy link
Member

Choose a reason for hiding this comment

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

the changes to this file should mostly be reverted (just the change to js-library-detector should be in here since it has no deps). I think this happens with an out of date yarn version, maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah I think this was because of an old version on my machine stripping all the integrity entries.

Fixed!

@@ -214,6 +209,11 @@ const expectations = [
description: 'Failed to load resource: the server responded with a status of 404 (Not Found)',
url: 'http://localhost:10200/dobetterweb/unknown404.css?delay=200',
},
{
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't expect these error messages to change due to this PR (this may be the cause of one of the smoke test failures)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep this was failing one of the tests, and i'm not even sure how this changed 😅

Reverted

@housseindjirdeh
Copy link
Contributor Author

I assumed all the smoke tests were being run with yarn test and didn't know there was a separate script for it. There were a few failing so thanks for spotting @brendankenny

I'm not sure how far down this path we want to go (since at some point it'll become more a test of JS LIbrary detector than of Lighthouse), but a basic multi-stack test doesn't seem like a ton of overhead.

That sounds good! Will definitely have this in a follow-up PR. A test for all the stacks we currently have definitely sounds like a good start, but agreed not sure if it would be worthwhile to keep adding more and more.

the other smoke test failure comes from what looks like the update to js-library-detector now also having a "jQuery (Fast path)" test

Ah yes, a few "fast path" versions of stacks were added including jQuery. Added to the expectations 👍

@housseindjirdeh
Copy link
Contributor Author

I also see a failing byte-efficiency test 50% of the time and I don't think any of my changes would affect this 🤔. I'm assuming this can happen when LH has a flaky run on my machine when running smoke tests?

Screen Shot 2019-10-15 at 11 34 01 AM

@@ -14,6 +14,8 @@ const expectations = [
artifacts: {
Stacks: [{
id: 'jquery',
}, {
id: 'jquery', // for jquery's "fast path" stack. Multiple stacks have a second "fast path" version in the library detector
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove comment and add name: 'jQuery (Fast path)' ?

Copy link
Member

Choose a reason for hiding this comment

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

remove comment and add name: 'jQuery (Fast path)' ?

yeah, that might be simpler than my comment idea :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

@@ -14,6 +14,8 @@ const expectations = [
artifacts: {
Stacks: [{
id: 'jquery',
}, {
id: 'jquery', // for jquery's "fast path" stack. Multiple stacks have a second "fast path" version in the library detector
Copy link
Member

Choose a reason for hiding this comment

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

remove comment and add name: 'jQuery (Fast path)' ?

yeah, that might be simpler than my comment idea :)

@@ -1839,6 +1839,26 @@
"detector": "js",
"id": "wordpress",
"name": "WordPress"
},
{
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we want to artificially add these to sample_v2 (we can wait for the follow up smoke test). yarn update:sample-artifacts Stacks (then yarn update:sample-json again) should update this file to what's really in dbw_tester (should be jquery + jquery fast path + wordpress)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah done, removed and re-updated

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

LGTM! 📚📦

@brendankenny brendankenny changed the title core(stack-packs): fresh new packs core: add angular, react, amp, and magento stack packs Oct 15, 2019
@brendankenny brendankenny changed the title core: add angular, react, amp, and magento stack packs core(stack-packs): add angular, react, amp, and magento packs Oct 15, 2019
@brendankenny brendankenny merged commit 21c8d2f into GoogleChrome:master Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants