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

feat: add npm query cmd #5000

Merged
merged 9 commits into from Aug 1, 2022
Merged

feat: add npm query cmd #5000

merged 9 commits into from Aug 1, 2022

Conversation

ruyadorno
Copy link
Collaborator

Implemented Arborist querySelectorAll method that enables easier
retrieval of installed packages using a css-selector-like syntax.

It also includes a new command npm query that exposes that same logic
to users of the npm cli.

References

Original RFC: npm/rfcs#564

@ruyadorno ruyadorno requested a review from a team as a code owner June 8, 2022 21:58
@ruyadorno ruyadorno added semver:minor new backwards-compatible feature Agenda will be discussed at the Open RFC call ws:arborist Related to the arborist workspace labels Jun 8, 2022
@npm-cli-bot
Copy link
Collaborator

npm-cli-bot commented Jun 8, 2022 •
edited

no statistically significant performance changes detected

timing results
app-large clean lock-only cache-only cache-only
peer-deps		 modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 44.300 ±3.62 20.016 ±0.09 18.084 ±0.14 20.685 ±0.65 3.201 ±0.01 3.224 ±0.06 2.619 ±0.05 12.445 ±0.03 2.541 ±0.02 3.843 ±0.17
#5000 41.589 ±2.13 20.065 ±0.12 18.090 ±0.37 20.792 ±0.41 3.198 ±0.04 3.244 ±0.07 2.633 ±0.01 12.636 ±0.08 2.559 ±0.00 3.824 ±0.09
app-medium clean lock-only cache-only cache-only
peer-deps		 modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 30.329 ±0.21 15.290 ±0.29 13.410 ±0.17 14.504 ±0.47 2.908 ±0.02 2.936 ±0.01 2.618 ±0.05 8.994 ±0.07 2.432 ±0.01 3.402 ±0.10
#5000 31.285 ±0.52 15.605 ±0.33 13.659 ±0.15 14.600 ±0.18 2.969 ±0.02 2.936 ±0.05 2.620 ±0.01 9.074 ±0.21 2.457 ±0.02 3.306 ±0.01

@ruyadorno ruyadorno force-pushed the ruyadorno/npm-query branch 5 times, most recently from 0fb004a to 4511d0d Compare June 10, 2022 16:42
ljharb
ljharb reviewed Jun 10, 2022
View reviewed changes
lib/utils/query-selector-all-response.js Outdated Show resolved Hide resolved
workspaces/arborist/lib/query-selector-all.js Outdated Show resolved Hide resolved
workspaces/arborist/lib/query-selector-all.js Outdated Show resolved Hide resolved
workspaces/arborist/lib/query-selector-all.js Outdated Show resolved Hide resolved
workspaces/arborist/lib/query-selector-all.js Outdated Show resolved Hide resolved
workspaces/arborist/lib/query-selector-all.js Outdated Show resolved Hide resolved
workspaces/arborist/lib/query-selector-all.js Outdated Show resolved Hide resolved
workspaces/arborist/lib/query-selector-all.js Outdated Show resolved Hide resolved
@fritzy fritzy mentioned this pull request Jun 15, 2022
Closed
@fritzy
Copy link
Contributor

fritzy commented Jun 19, 2022

@darcyclarke I would love to know more about bugs you've mentioned encountering

@wraithgar wraithgar assigned wraithgar and unassigned fritzy Jun 20, 2022
@wraithgar wraithgar force-pushed the ruyadorno/npm-query branch 2 times, most recently from 276da62 to 06e91a8 Compare June 22, 2022 15:39
@darcyclarke darcyclarke mentioned this pull request Jun 22, 2022
Closed
ljharb
ljharb reviewed Jun 22, 2022
View reviewed changes
workspaces/arborist/lib/query-selector-all.js Outdated Show resolved Hide resolved
workspaces/arborist/lib/query-selector-all.js Outdated Show resolved Hide resolved
workspaces/arborist/lib/query-selector-all.js Outdated Show resolved Hide resolved
@wraithgar wraithgar force-pushed the ruyadorno/npm-query branch 2 times, most recently from d2c7c33 to 319c837 Compare June 23, 2022 20:03
ruyadorno
ruyadorno commented Jun 28, 2022
View reviewed changes
Copy link
Collaborator Author

@ruyadorno ruyadorno left a comment

Choose a reason for hiding this comment

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

awesome stuff @wraithgar!

I'm curious about the perf issues, I see you were using p-map before and I thought the improvements were due to that but it's no longer being used atm 🤔

test/lib/commands/query.js Outdated Show resolved Hide resolved
workspaces/arborist/lib/query-selector-all.js Outdated Show resolved Hide resolved
@wraithgar
Copy link
Member

awesome stuff @wraithgar!

I'm curious about the perf issues, I see you were using p-map before and I thought the improvements were due to that but it's no longer being used atm 🤔

Nothing under the hood was doing anything asynchronously. Removing async functions altogether for functions that were completely synchronous made it faster. We're also in the "make it right" phase. Performance isn't really an issue right now, it takes less than a second to query the cli tree. We can worry about performance next, after we "make it right".

@wraithgar wraithgar force-pushed the ruyadorno/npm-query branch 3 times, most recently from 0345c40 to bde355b Compare June 29, 2022 15:49
@wraithgar
Copy link
Member

Refactor completed. No more melting cpus on workspace / has queries. include-workspace-root flag is obeyed. Lots of other cleanups and code consolidations. This may be ready to go.

@wraithgar
chore: refactor tests to use real npm
4332744
@wraithgar
deps: @npmcli/query@1.1.0
4e8a00f
@wraithgar
fix: attribute selectors, workspace root, cleanup
eccf71b 
Giant refactor:
  - adds case insensitivity to attribute selectors
  - fixes --include-workspace-root
  - fixes -w results
  - docs updates
  - consolidating state into the `results` object and passing that to
    the functions that the ast walker functions use.
  - optimizing and refactoring other loops
  - code consolidation and consistency between two different attribute
    selectors
  - Un-asyncify functions that don't do async operators.  We leave the
    exported fn async so we can add some in the future.
  - lots of other minor tweaks/cleanups
@wraithgar wraithgar force-pushed the ruyadorno/npm-query branch 8 times, most recently from 8a9a7cd to a1ed199 Compare July 18, 2022 18:19
This was referenced Jul 20, 2022
Closed
Closed
@ruyadorno
Copy link
Collaborator Author

ruyadorno commented Jul 27, 2022 •
edited

Refactor completed. No more melting cpus on workspace / has queries. include-workspace-root flag is obeyed. Lots of other cleanups and code consolidations. This may be ready to go.

@wraithgar I'm curious on what was the issue with the melting cpus in the original implementation of the has queries 😅 How did it got fixed?

@wraithgar
Copy link
Member

@ruyadorno I'm not 100% certain to be honest. iirc there were a couple of places where the wrong values where being passed into retrieveNodesFromParsedAst. It was some kind of infinite loop that wasn't also infinite recursion. All I know for certain was that it was in rolling up all state into the Results object that the bug was fixed.

@ruyadorno
Copy link
Collaborator Author

ruyadorno commented Jul 27, 2022 •
edited

I see, sounds good! thanks for taking care and cleaning up all that 👍 I'm looking forward to this release 🤩

@nlf nlf merged commit 3c024ac into latest Aug 1, 2022
@nlf nlf deleted the ruyadorno/npm-query branch August 1, 2022 18:29
@github-actions github-actions bot mentioned this pull request Aug 1, 2022
Merged
@ruyadorno
Copy link
Collaborator Author

🥳

@darcyclarke darcyclarke mentioned this pull request Aug 3, 2022
Closed
@nlf nlf mentioned this pull request Aug 3, 2022
Merged
@npm-cli-bot npm-cli-bot mentioned this pull request Aug 3, 2022
Merged
@darcyclarke darcyclarke removed the Agenda will be discussed at the Open RFC call label Aug 3, 2022
@lorenzogrv
Copy link

Nice work guys. Thank you all 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wraithgar wraithgar wraithgar left review comments

@ljharb ljharb ljharb left review comments

Assignees

@wraithgar wraithgar

Labels
semver:minor new backwards-compatible feature ws:arborist Related to the arborist workspace
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@ruyadorno @npm-cli-bot @fritzy @wraithgar @lorenzogrv @ljharb @nlf @darcyclarke