Skip to content

misc: rename 'lighthouse-' folders #12859

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.

Sign up for GitHub

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

Jump to bottom
Closed
connorjclark wants to merge 5 commits into from
Closed

misc: rename 'lighthouse-' folders #12859

connorjclark wants to merge 5 commits into from

Conversation

@connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Aug 4, 2021 •
edited
Loading 

git mv lighthouse-core core
git mv lighthouse-cli cli
git mv lighthouse-viewer viewer
git mv lighthouse-treemap treemap
git mv lighthouse-logger logger
  • global find/replace for lighthouse-core -> core, etc. (except for lighthouse-logger)
  • don't change lighthouse-viewer in firebase-auth.js (api credentials)
  • manual fixing of .md files (some links are pointing to particular commits)
  • ignored changelog.md

This is a huge change, but since the only manual aspect of it was the *.md files you can see a smaller diff via git diff origin/master origin/great-move -- '*.md'. Our extensive tests should give us confidence to make this change.

It just occurred to me that changing the location of (lighthouse-)core/config/lr-desktop-config.js used for --config-path may break someone. Should we do a fallback config lookup in bin.js if .../core/config/... doesn't match anything (try .../lighthouse-core/config/...)? And possibly drop that next major version.

@connorjclark connorjclark requested a review from a team as a code owner August 4, 2021 22:34
@connorjclark connorjclark requested review from adamraine and removed request for a team August 4, 2021 22:34
@google-cla google-cla bot added the cla: yes label Aug 4, 2021
@brendankenny
Copy link
Contributor

brendankenny commented Aug 4, 2021

I applaud the Herculean effort but this might need to be a breaking change :/

We haven't always done the best job exposing the API end points folks need, so e.g. just a cursory search of pubads there's a bunch of places they reach into lighthouse-core/ and a few to lighthouse-cli/.

Viewer and treemap might be workable, though?

@connorjclark
Copy link
Collaborator Author

connorjclark commented Aug 4, 2021

We haven't always done the best job exposing the API end points folks need, so e.g. just a cursory search of pubads there's a bunch of places they reach into lighthouse-core/ and a few to lighthouse-cli/.

Obvious in hindsight, but strangely I can't get this error to happen locally with yarn build-devtools, so I'm stuck debugging a solution in GHA. I'd bet that only pubads is using anything beyond require('lighthouse'), and so we have the possibility of just fixing it ourselves without breaking any existing plugins. Just a hunch, I'll take a look at existing plugins.

@brendankenny
Copy link
Contributor

brendankenny commented Aug 4, 2021 •
edited
Loading

It's not just pubAds, lighthouse-ci does too. I'm sure most many integrations do.

@patrickhulce
Copy link
Collaborator

patrickhulce commented Aug 5, 2021

+1 to this being a breaking change.

While Lighthouse CI does indeed do this, we're pinned to a very specific Lighthouse version such that we could probably handle it OK. My greater concern is that anyone using the report renderer would be in the same position and probably didn't peg to a single version.

@connorjclark
Copy link
Collaborator Author

connorjclark commented Aug 5, 2021

See y'all here in a few months :)

This was referenced Oct 21, 2021
Merged
Merged
@connorjclark connorjclark mentioned this pull request Jul 27, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adamraine adamraine Awaiting requested review from adamraine

Assignees

@adamraine adamraine

Labels

cla: yes waiting4reviewer

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@connorjclark @brendankenny @patrickhulce @adamraine @devtools-bot