Allow running lighthouse on other pages available in publish folder

[aitchiss]

Allows users to target pages in the given directory other than index.html. Also removes the need to pass an empty string for path if targeting the top-level serve directory.

Closes https://github.com/netlify/pillar-workflow/issues/860

[netlify]

👷 Deploy Preview for plugin-lighthouse processing.

Name	Link
🔨 Latest commit	4ffdf4b
🔍 Latest deploy log	https://app.netlify.com/sites/plugin-lighthouse/deploys/6340224224486e00082ef161

[kevinsamoei]

LGTM!

[jackbrewer]

Code looks great ☺️

Regarding the README, I feel like the reasoning for having this new option isn't explained as much as it could be, and the option name could maybe be tweaked (disclaimer, it's still confusing to me!)

I thought this work was achieving something different than being able to directly address (html) files, although this work does also fix the problem I though we were aiming for so it's all looking good!

Example

Before: This 11ty site deploy – if you go to the second lighthouse report and look at the report, it mentions missing styles/scripts etc. Also note the scores aren't great. This was run with path = posts/. As express is serving the /posts page directly as the site root (this is what the path currently means), links to assets outside the posts directory break.

After: This follow-up deploy was run with fileName = posts/ (no path set), and the second report in that deploy now has all assets available, the screenshots look fixed, and the scores are back up where they should be. Express is running the site from the root directory, then we're looking into the sites posts directory using fileName. All the asset paths are correct and reachable as the site is running from the root directory. A valid fileName value could be posts/category/date/title, which doesn't fit too well with being a "file name"

I think:

• path = where should I start up the server
• fileName = from path, where shall I look for the actual file to test – does "fileName" still work here?

[jackbrewer]

Urgh, I meant to comment, not reject. Happy to resubmit as approved if you'd rather update docs etc as a follow-on

[jackbrewer]

Now I think about it more, should we be de-emphasising the use of path, and making filePath the primary option mentioned in README/docs. I imagine it's a lot more useful in most cases, and actually does what most people assume path does.

[aitchiss]

This is all great context thank you @jackbrewer!

I think:

path = where should I start up the server
fileName = from path, where shall I look for the actual file to test – does "fileName" still work here?

Yes I think this is the right distinction, and no I don't think the current names reflect it well 😂

What I would like to change these to is:

serveDir = what to serve
path = where to look inside to test - could be a dir, could be a specific html file

What do you think? It feels a bit wrong that path is being shifted to a new meaning, but I don't think there's any negative consequence of doing that (and updating the documentation to match). If anything, it's bringing things more inline with what you'd expect (like the 11ty example you linked above).

serveDir would probably become a less-used option.

[nasivuela]

What I would like to change these to is:

serveDir = what to serve
path = where to look inside to test - could be a dir, could be a specific html file

What do you think? It feels a bit wrong that path is being shifted to a new meaning, but I don't think there's any negative consequence of doing that (and updating the documentation to match). If anything, it's bringing things more inline with what you'd expect (like the 11ty example you linked above).

serveDir would probably become a less-used option.

I also think this approach is clearer!
Changing the meaning of path will be an improvement. As long as we document it and bump the major version to reflect the breaking change I do not see any negative consequence.

[jackbrewer]

Looking good 😄

[jackbrewer]

@aitchiss I noticed this appearing in the deploy logs, not sure if it was intentional?! I didn't spot it in the review 🙃

[aitchiss]

🙈