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

RFC: Lighthouse Subpackage Proposal (report UI and tracehouse) #9519

Closed
patrickhulce opened this issue Aug 6, 2019 · 16 comments
Closed

RFC: Lighthouse Subpackage Proposal (report UI and tracehouse) #9519

patrickhulce opened this issue Aug 6, 2019 · 16 comments
Assignees
Labels

Comments

@patrickhulce
Copy link
Collaborator

patrickhulce commented Aug 6, 2019

EDIT: Updated June 2020 with the elements of this that we no longer need styled with strikethrough.

Summary

Long-term goals:

  • Ensure package-like parts of Lighthouse are easy to maintain, develop, and consume.
  • Facilitate the improvement of report UI components to handle use cases outside of the core report and a more app-like experience remains embeddable.

Long-term Proposal:

  • Structure files/folders in a psuedo-lerna packages/* style with core, cli, report, viewer, tracehouse, logger to establish logical delineations between packages.
  • Publish lighthouse-logger from packages/logger and publish tracehouse from packages/tracehouse, continue to publish the root as lighthouse initially.
  • Eventually publish subpackages of core, cli, and report.
  • Other package-like things (dependency graph, sd validation, manifest parser, etc) are first organized in a folder under packages/core/lib before being promoted to a folder in packages/ if demand exists.

Short-term goals:

  • Share report UI elements with lighthouse-ci MVP.
  • Regularly publish tracehouse as its own package.

Short-term Proposal:

  • Break apart the monolithic templates.html and report-styles.css into reusable web components that could be require'd in lighthouse-ci.
  • Augment those web components to support diff-modes where applicable.
  • Do the file/folder restructuring above and publish a minimal API from packages/tracehouse.

Short-term Hackier Alternatives:

  • Fork just the necessary report UI components in the lighthouse-ci repo, copy/paste certain styles for consistency.
  • Publish tracehouse straight from lighthouse-core/lib/tracehouse. It's dependencyless and I mean how frequently do we update that logic anyhow.

To be honest, given our timeframe constraints and lack of clarity on where the report UI components might go, I think I prefer the hackier alternatives, but the long-term vision of doing things the right way does get me all warm and fuzzy, so feedback welcome! :)

@brendankenny
Copy link
Member

I'll try to leave my grumpiness for all subpackages at the door here :)

There is a large tradeoff here with the flexibility and inter-code visibility that something closer to a monorepo allows. Componentizing things can improve maintainability and local understandability, but as more of those components are published separately the larger our committed public interface, the more complicated our release process grows, the harder cross-component refactors are, etc.

No one likes to deal with lighthouse-logger or chrome-launcher for a (multiple) reason(s), and those two packages also stand as a good example of how once this path is committed to there's basically no going back, as least without a lot of work that won't ever rise in priority vs just living with the pain and getting other stuff done.

core/ and cli/ are another good example where the line between the two has been redrawn multiple times, and had we committed to a particular interface for the both of them and published them a year or two ago there are several refactors as our number of clients has grown that probably wouldn't have been possible.

Publish tracehouse straight from lighthouse-core/lib/tracehouse. It's dependencyless and I mean how frequently do we update that logic anyhow.

let's ask @patrickhulce: https://github.com/GoogleChrome/lighthouse/pulls?utf8=%E2%9C%93&q=is%3Apr+tracehouse :P

(more seriously for tracehouse, you and @paulirish have convinced me it's inevitable so that sounds fine :)

I think the report stuff is going to be the biggest win for our current needs, and these:

  • Break apart the monolithic templates.html and report-styles.css into reusable web components that could be require'd in lighthouse-ci.
  • Augment those web components to support diff-modes where applicable.

sound like a good approach. In particular how to compose a Lighthouse report UI that

  • looks like what we want it to, including the kind of customizations that required performance-category-renderer and the pwa-category-renderer to be split off from the base renderer
  • can be redesigned without a lot of overhead beyond what it would take to redesign any website
  • is flexible and encapsulated enough that parts of it can be embedded in other pages (like ci and web.dev)
  • isn't so flexible that it becomes essentially another templating layer and the different report clients have to write what amounts to a custom report-renderer each anyway

this sounds hard, and the more we add customized flourishes to the report it sounds harder, but I think would be super valuable.

@patrickhulce
Copy link
Collaborator Author

patrickhulce commented Aug 6, 2019

how frequently do we update that logic anyhow.

Will address the rest later, but here I was talking about the core of the trace processing logic. There's obviously a lot of iterative work that is going into making tracehouse a distinct thing, but prior to that flurry of activity most lines blamed to 2-3 years ago and the most recent commit was 10 months ago. https://github.com/GoogleChrome/lighthouse/blame/9f805249c2a019271fb19c0d8e68db0d93fc540a/lighthouse-core/lib/tracehouse/trace-of-tab.js

@patrickhulce
Copy link
Collaborator Author

I'm reviving this thread because it's become relevant again for tracehouse :) I updated the original issue striking through a few things that are no longer relevant but the rest of the bones are solid.

I'll start here with a description of how things work in Lighthouse CI which uses a monorepo structure.

Lighthouse CI is comprised of 4 packages that are published independently but versioned synchronously, i.e. all packages latest version is always the same, a breaking change in one package results in a breaking change version bump to all the packages.

  • @lhci/cli - the user-facing library consumers use in CI, analogous to the lighthouse package today but without the programmatic node API
  • @lhci/server - the user-facing library used
  • @lhci/viewer - the mostly internal but also user-facing library for the standalone LHR comparison view that powers https://googlechrome.github.io/lighthouse-ci/viewer/
  • @lhci/utils - the internal library for shared code between all three packages

Each subpackage is contained to a folder in packages/ and dependencies are expressed through the package.json like normal. For the most part you could consider each individual package folder as if it were its own repository with it's own dependencies, tests, scripts, etc. Yarn workspaces assists with duping the shared devDependencies, speeding up installs, and sharing code.

Development Experience

At development time, I have not experienced any substantive differences compared to if these were all in some massive single package like lighthouse. Yarn workspaces makes this fairly seamless by creating symlinks in node_modules/@lhci/<pkg> to packages/<pkg> so all requires in code can be written normally (@lhci/cli can just require('@lhci/utils') yet still use the latest code on disk.

Release Experience

Release process is extremely simple and completely automated by script with the assistance of lerna. All package.json versions have 0.1.0 on disk until publish time at which point the script looks up the last git tag version/what was published to NPM and updates the version to the next version at publish time. This eliminates the need for Bump the version to X.X.X commits/PRs Unfortunately, this works well for a reason that is not really shared with lighthouse/tracehouse.

These packages in LHCI are all tightly coupled. A breaking change in the server generally requires a breaking change to the client, so it is unusual that a consumer would ever not upgrade these in unison anyway. Syncing the versions greatly simplifies the release process and makes this process automatic. This is probably not going to be true of lighthouse/tracehouse and definitely wasn't true of chrome-launcher, so more thought will need to go into this.

Applicability to Lighthouse

I see no reason why the day-to-day development experience would be any different in Lighthouse than today with a monorepo structure. The external contributor experience is not exactly smooth today and smaller scoped packages invite the possibility of narrowing the required setup or understanding to make changes while not meaningfully increasing any burden of setup beyond usage of yarn.

IMO, the biggest challenge for bringing the LHCI style is going to be figuring out a versioning policy, automating releases, and clearly communicating changelog. If we decide to keep distinct version tracks for each subpackage, things will be very challenging. If we don't, then our API surface for what requires a breaking change becomes considerably larger and more difficult to manage as @brendankenny alluded to.

I also see i18n being a significant challenge if we end up splitting strings across package boundaries. I think it'd be reasonable to move forward with the assumption that all i18n strings must remain in the core package for now.

Request for Comment

  1. Are folks excited about this idea for reorganization?
  2. Can we agree on a subset of packages to focus on? I'm thinking lighthouse, lighthouse-logger, and tracehouse.
  3. Does anyone think we could commit to keeping major versions aligned between our subpackages? If we don't, how would we document/communicate/tag/release notes/changelog/etc the different versions of different pacakges?
  4. Is this at all worth it or should I focus on another solution for tracehouse like a git mirror of lighthouse-core/lib/tracehouse I maintain with a few scripts in a separate repo?

Honestly, if no one else is excited about this sort of change, then I'd probably lean toward the solution in question 4 and not bother pushing this boulder any further up the hill :)

@brendankenny
Copy link
Member

  1. Are folks excited about this idea for reorganization?

I am not excited about extra indirection and complexity introduced to keep packaging systems happy, BUT I am excited about streamlining packaging and eliminating special cases (like lighthouse-logger) and working on reducing those not exciting things, so I think this is worth serious consideration.

If we decide to keep distinct version tracks for each subpackage, things will be very challenging

Yeah, so if we say, added support for finding LCP events in the trace and wanted to release that right away in tracehouse, but then didn't do a Lighthouse release for a while after that, how would that look? That seems like it would be ok-ish (would lighthouse's tracehouse dep version get bumped when tracehouse was released or does it not matter and it always uses the local version?).

What other dependency situations are tricky?

Other things:

  • Maybe we should have a file path traversal length metric between files and try to find an arrangement that minimizes that :) My understanding is that the symlinks work for node_modules but humans will still have to click through a bunch of steps in github or in their OS file browser the farther each source file gets from root (meanwhile the dotfiles that computers care about get to multiply where they're most prominent for humans 😢)
  • Should we wait until npm supports workspaces? Does lhci support npm installation and lerna handles it in that case or is it yarn only? Supposedly yarn-compatible workspaces are coming in npm v7, though that blog post is from a long time ago and obviously there have been some changes over there since then (ooh, and they plan on supporting yarn.lock as the source of truth if it already exists)

@patrickhulce
Copy link
Collaborator Author

didn't do a Lighthouse release for a while after that, how would that look?

I'm not sure which aspect of "look" you're most interested in here. If we diverge on version between packages, I don't have a final answer for the communication/tagging/release notes/changelog pieces that's partly what's up for discussion. But I imagine we store the version in package.json and create a script that...

  1. Collects all commits with scope tracehouse: for changelog and release notes.
  2. Creates a commit that bumps the version in the package.json and bumps the dependent package.json versions, writes to the changelog.

Another script would run post-merge that

  1. Tags the version bump commit on master with <pkg>-<version>.

I would maybe only expect GitHub releases to be created for lighthouse proper?

would lighthouse's tracehouse dep version get bumped when tracehouse was released or does it not matter and it always uses the local version

It would always use the local version. This increases the importance of the release test script testing the CLI with real npm installed dependenices outside the repo.

My understanding is that the symlinks work for node_modules but humans will still have to click through a bunch of steps in github or in their OS file browser the farther each source file gets from root

I'm not sure I understand this concern given that anything we're proposing is trying to pull code out of the depths of core into it's own high level thing. If you're referring to the fact that there is a packages/ prefix. That's just convention and if it's really not appealing then package folders could be stored at root, not a big deal.

Should we wait until npm supports workspaces?

Do we really support npm now? All of our packge.json scripts for testing use yarn explicitly. It's not like one could meaningfully contribute to lighthouse today without yarn already. If it's the situation where one is not running any tests then I'd argue this actually improves the situation since you can ignore the monorepo and just run npm install directly in the package the contributor is working on and ignore everything else.

Does lhci support npm installation and lerna handles it in that case or is it yarn only?

Lerna is not used or referenced in LHCI development at all, lerna is exclusively used at publish time. yarn workspaces are required.

@connorjclark
Copy link
Collaborator

Can we agree on a subset of packages to focus on? I'm thinking lighthouse, lighthouse-logger, and tracehouse.

I was initially confused and thought you wanted a package each for cli and core. I'm against that very much. But based on this comment that isn't your intention, correct?

@patrickhulce
Copy link
Collaborator Author

I was initially confused and thought you wanted a package each for cli and core.

That is a long-term goal I believe would be great. I'm very curious to hear why you'd be against it very much :)

But based on this comment that isn't your intention, correct?

For the first step of subpackage management correct I would not prioritize this. If we see no world in which we would split out the other packages though, then I would not be in favor of this work. I'd prefer to just expose tracehouse via option in bullet 4.

@connorjclark
Copy link
Collaborator

cli@x would have a direct dependency on core@x? i guess I actually have no issue with that nvm.

@patrickhulce
Copy link
Collaborator Author

We've decided to just add a few lightweight scripts and separate package.jsons to deploy lighthouse-core/lib/tracehouse and a bundled report renderer separately without much formal subpackage structure/git tagging/etc.

If there's appetite for more and/or usage of these is high we can consider revisiting this approach in the future.

@dvelasquez
Copy link
Contributor

We've decided to just add a few lightweight scripts and separate package.jsons to deploy lighthouse-core/lib/tracehouse and a bundled report renderer separately without much formal subpackage structure/git tagging/etc.

If there's appetite for more and/or usage of these is high we can consider revisiting this approach in the future.

Can we try to do the same with the viewer? I'm willing to help on this.

@patrickhulce
Copy link
Collaborator Author

@dvelasquez I believe what you've been referring to as "viewer" is what we call the report renderer.

Viewer means something very different to us as it's a standalone app hosted on Github pages that has firebase auth, github authentication. Github gist storage, GA tracking, multiple lighthouse version support back to v2, etc. We do not intend on packaging this app for npm.

@dvelasquez
Copy link
Contributor

Cool, I see the difference now @patrickhulce
Do you recommend to still use the report renderer as the source to make the wrappers I have? https://github.com/dvelasquez/lighthouse-viewer

@patrickhulce
Copy link
Collaborator Author

Yes for the purposes of your project the report renderer source is exactly what you want 👍

@adrianaixba
Copy link
Collaborator

@patrickhulce is this proposal still pending? 👀

@patrickhulce
Copy link
Collaborator Author

Ah yes @adrianaixba I'm slowly working on this for tracehouse ( #11034 #11253 ), I'll update :)

@connorjclark
Copy link
Collaborator

We did some inter-project organization recently (report, shared, ...) that improved some things here, but there hasn't been any motivation recently to go all-in on separate npm packages. Going to close for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants