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

clients: reorganize file structure and build process #6344

Merged
merged 4 commits into from
Oct 25, 2018
Merged

Conversation

brendankenny
Copy link
Member

fixes #6299. New file structure matches the latest version of that issue, recreated here for reference:

├── clients/
│   ├── devtools-entry.js   (could be in subdirectory but seems unnecessary)
│   ├── lightrider-entry.js
│   ├── extension/   (current top-level lighthouse-extension/)
│   |   ├── scripts/ (currently src/)
│   |   ├── styles/
│   |   ├── manifest.json
│   |   ├── popup.html
│   |   └── etc...
│   └── test/
│       ├── lightrider-entry-test.js
│       └── extension/
│          ├── extension-test.js
│          └── popup-test.js
├── build/
│   ├── build-extension.js
│   ├── bundle-builder.js
│   └── changelog-generator/   (already exists)
├── dist/   (created while building)
│   ├── lighthouse-dt-bundle.js
│   ├── lighthouse-lr-bundle.js
│   ├── extension/
│   |   └── scripts/, styles/, manifest.json, etc
│   └── extension-package
│          ├── lighthouse-3.2.0.zip
│          └── lighthouse-3.2.1.zip
├── lighthouse-cli/
├── lighthouse-core/
└── etc...

@brendankenny
Copy link
Member Author

damn, pr lint change didn't work on itself :)

@brendankenny
Copy link
Member Author

And not too late to bikeshed if something doesn't look right to you now

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. qq: any reason it is build-all, but then test-clients? Not: test-all, or build-all-clients? To be consistent.

@brendankenny
Copy link
Member Author

qq: any reason it is build-all, but then test-clients?

just dumb reasons. We could follow up by making viewer a client (it arguably is, bundling just the report generator and renderer), then it would be build-clients. I don't think we'd want it to be test-all, because that would really imply running all the tests, including over core, which yarn test sort of already handles (though it doesn't run integration tests like smokehouse, but these client tests are mostly integration tests via puppeteer...).

Meanwhile, why build-all (or build-clients) and not just build? I think because install-all came first, which was an effort to differentiate from install because we didn't want to install all deps by default. That's still kind of a concern (though the original, main reason for doing that has long disappeared), but it might be worth revisiting to simplify things.

So, all in all, someone needs to get in there and make our npm scripts sane. 55 down to like 40 or so should maybe be the goal :)

@paulirish paulirish changed the title clients: reorganize file structure and build process clients: reorganize file structure and build process Oct 25, 2018
@paulirish
Copy link
Member

You'll want to manually delete lighthouse-extension/ from your checkouts.

@paulirish paulirish merged commit 3b562ab into master Oct 25, 2018
@paulirish paulirish deleted the clients branch October 25, 2018 23:11
Copy link

@bintang992 bintang992 left a comment

Choose a reason for hiding this comment

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

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

Successfully merging this pull request may close these issues.

Reorganize bundles and buildles
4 participants