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.

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

core(stacks): match against js-lib-detector with id property #9888

Merged
merged 4 commits into from
Oct 30, 2019
Merged

core(stacks): match against js-lib-detector with id property #9888

merged 4 commits into from
Oct 30, 2019

Conversation

housseindjirdeh
Copy link
Contributor

Bugfix: #9881

@housseindjirdeh housseindjirdeh mentioned this pull request Oct 25, 2019
Closed
paulirish
paulirish approved these changes Oct 25, 2019
View reviewed changes
Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

works for me, thanks!

lighthouse-core/lib/stack-collector.js Outdated
@@ -53,6 +55,7 @@ async function detectLibraries() {
const result = await lib.test(window);
if (result) {
libraries.push({
id: lib.id,
name: name,
icon: lib.icon,
Copy link
Member

Choose a reason for hiding this comment

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

we can drop icon from here now

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 👍

@paulirish
Copy link
Member

@housseindjirdeh you got a few failures in CI

can you run yarn to update the lockfile?

also can you update the PR title? core(js-libs): match on id property should do it

@brendankenny brendankenny changed the title Connects JS libraries via id core: categorize stacks strictly by id Oct 29, 2019
brendankenny
brendankenny approved these changes Oct 29, 2019
View reviewed changes
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.

LGTM2

can you run yarn to update the lockfile?

yeah, looks like a yarn should fix things, and the smoke test will need to be updated to use the new ids

@housseindjirdeh housseindjirdeh changed the title core: categorize stacks strictly by id core(js-libs): match on id property Oct 29, 2019
@brendankenny brendankenny changed the title core(js-libs): match on id property core(stacks): match on id property Oct 29, 2019
@housseindjirdeh
Copy link
Contributor Author

Updated (both the lockfile and smoke test with the new id)

@paulirish paulirish changed the title core(stacks): match on id property core(stacks): match against js-lib-detector with id property Oct 30, 2019
@paulirish paulirish merged commit 3f74f33 into GoogleChrome:master Oct 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brendankenny brendankenny brendankenny approved these changes

@paulirish paulirish paulirish approved these changes

@patrickhulce patrickhulce Awaiting requested review from patrickhulce

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@housseindjirdeh @paulirish @brendankenny @googlebot