-
-
Notifications
You must be signed in to change notification settings - Fork 643
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
Created an NpxToolBase
as an inheritable Subsystem for nodejs
tools
#17567
Merged
sureshjoshi
merged 3 commits into
pantsbuild:main
from
sureshjoshi:17557-npx-dep-version
Nov 18, 2022
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
20 changes: 20 additions & 0 deletions
20
src/python/pants/backend/javascript/subsystems/npx_tool.py
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
# Copyright 2022 Pants project contributors (see CONTRIBUTORS.md). | ||
# Licensed under the Apache License, Version 2.0 (see LICENSE). | ||
|
||
from __future__ import annotations | ||
|
||
from typing import ClassVar | ||
|
||
from pants.option.option_types import StrOption | ||
from pants.option.subsystem import Subsystem | ||
|
||
|
||
class NpxToolBase(Subsystem): | ||
# Subclasses must set. | ||
default_version: ClassVar[str] | ||
|
||
version = StrOption( | ||
advanced=True, | ||
default=lambda cls: cls.default_version, | ||
help="Version string for the tool in the form package@version (e.g. prettier@2.6.2)", | ||
) |
41 changes: 41 additions & 0 deletions
41
src/python/pants/backend/javascript/subsystems/npx_tool_test.py
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
# Copyright 2022 Pants project contributors (see CONTRIBUTORS.md). | ||
# Licensed under the Apache License, Version 2.0 (see LICENSE). | ||
|
||
from __future__ import annotations | ||
|
||
import pytest | ||
|
||
from pants.backend.javascript.subsystems import nodejs | ||
from pants.backend.javascript.subsystems.npx_tool import NpxToolBase | ||
from pants.testutil.rule_runner import QueryRule, RuleRunner | ||
|
||
|
||
class CowsayTool(NpxToolBase): | ||
options_scope = "cowsay" | ||
name = "Cowsay" | ||
# Intentionally older version. | ||
default_version = "cowsay@1.4.0" | ||
help = "The Cowsay utility for printing cowsay messages" | ||
|
||
|
||
@pytest.fixture | ||
def rule_runner() -> RuleRunner: | ||
return RuleRunner( | ||
rules=[ | ||
*nodejs.rules(), | ||
*CowsayTool.rules(), # type: ignore[call-arg] | ||
QueryRule(CowsayTool, []), | ||
], | ||
) | ||
|
||
|
||
def test_version_option_overrides_default(rule_runner: RuleRunner): | ||
rule_runner.set_options( | ||
[ | ||
"--backend-packages=['pants.backend.experimental.javascript']", | ||
"--cowsay-version=cowsay@1.5.0", | ||
] | ||
) | ||
tool = rule_runner.request(CowsayTool, []) | ||
assert tool.default_version == "cowsay@1.4.0" | ||
assert tool.version == "cowsay@1.5.0" |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to bikeshed on this name. NpxToolBase, NodeToolBase, NpmToolBase - I'm not sure which is the most precise, but extensible... Also may not matter, since it can always be changed later if need be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say NodeToolBase? We call the Python equivalent PythonToolBase, not PipToolBase or PyPIToolBase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is somewhat complicated -- I suggest
NpxToolBase
, asnpx
is a tool launcher, andnode
is not (it's an arbitrary script launcher).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like that would line up more with
PythonToolBase
in that case.The only reason I was thinking
NodeToolBase
is that fundamentally, all of the tools need to be NodeJS tools. However, the same logic applies tonpm
andnpx
since it's the same ecosystem and suite of tools.In Slack, @kaos mentioned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things launched by
yarn
come to mind as a good reason for being specific about the launcher.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Node is a runtime, so these are "tools that run on node" in the same way that PythonToolBases "run on the python interpreter" and JvmToolBases run on the JVM. So I think NodeToolBase is the right parallel?
Very roughly, Npx is to Node as Pip is to Python as Coursier is to JVM?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving
NpxToolBase
for now. Down the road when we think about potentially incorporatingyarn
,pnpm
, or whatever the flavour of the month is in the future - we can revisit.@benjyw
npx
isn't a package manager per se, but (more or less) an alias for "npm install" and then "npm exec" (https://docs.npmjs.com/cli/v7/commands/npx#npx-vs-npm-exec). So,npm
is thepip
.This line of thinking has gotten me to the conclusion that
NpxToolBase
is better for now, specifically because it brings with it one piece of implicit info. The package MUST be executable with abin
folder/file specified in thepackage.json
.For example:
npx dayjs
errors out withnpm ERR! could not determine executable to run
.So,
NpmToolBase
is incorrect, because the intention is an executable package. And for the same reason, I thinkNodeToolBase
is incorrect, as you can put in Node packages which aren't executable, and then thisToolBase
would be useless (without adding some extra flags, options, and shenanigans).