javascript: package yarn v2 #20540

kevinobruno · 2024-02-13T20:25:44Z

Add support for pants package using yarn@v2.

Currently when trying to package a yarn@v2 project, the following error occurs:

Engine traceback:
  in `package` goal

ProcessExecutionFailure: Process 'Installing [email protected].' failed with exit code 1.
stdout:
➤ YN0050: The --frozen-lockfile option is deprecated; use --immutable and/or --immutable-cache instead

stderr:

Thats because the option --frozen-lockfile was replaced with --immutable on yarn >= 2.

This behavior can be reproducible at https://github.com/kevinobruno/pants-yarn running pants package //yarnv2

kaos

thanks!

kaos · 2024-02-14T20:50:22Z

src/python/pants/backend/javascript/nodejs_project.py

    def immutable_install_args(self) -> tuple[str, ...]:
        if self.package_manager == "npm":
            return ("clean-install",)
+        if self.package_manager == "yarn":
+            if nodesemver.satisfies(self.package_manager_version, "1.x"):
+                return ("install", "--frozen-lockfile")
+            return ("install", "--immutable")
        return ("install", "--frozen-lockfile")


The trailing return here doesn't make sense to me. Do we expect any other package managers than "npm" or "yarn"? And if so, do we expect them to use intall --frozen-lockfile in that case?
How about raising an error to avoid proceeding with unknown results?

iiuc, pants also support pnpm. I kept the trailing return to not break existing uses of this backend, I also tried to follow the pattern of all other properties returning a default. Maybe we could check if the package manager is supported prior to the use of this property?

Ah, right. OK, so I'd prefer it if this method would be explicit about it then (regardless of how it was before, I think this could be improved from status quo) by not silently accepting "any" backend.

i.e. something along the lines of:

Suggested change

def immutable_install_args(self) -> tuple[str, ...]:

if self.package_manager == "npm":

return ("clean-install",)

if self.package_manager == "yarn":

if nodesemver.satisfies(self.package_manager_version, "1.x"):

return ("install", "--frozen-lockfile")

return ("install", "--immutable")

return ("install", "--frozen-lockfile")

def immutable_install_args(self) -> tuple[str, ...]:

if self.package_manager == "npm":

return ("clean-install",)

if self.package_manager == "yarn":

if nodesemver.satisfies(self.package_manager_version, "1.x"):

return ("install", "--frozen-lockfile")

return ("install", "--immutable")

if self.package_manager == "pnpm":

return ("install", "--frozen-lockfile")

raise E(f"Unsupported package manager: {self.package_manager}")

[..] Maybe we could check if the package manager is supported prior to the use of this property?

That's probably a good idea to give a nicer error message in one place. But I don't think I agree with returning default values unchecked. Imagine down the line adding a new package manager, then you have no guards that you've covered all bases as the default may not be appropriate.

Got it, changed to raise when an unsupported package manager is provided: 3fe7c6e.

I think there's abstractly a bit of tension here with being Pants being overly constraining.

For instance, what if someone publishes a pnpm-CLI-compatible cool-new-package-manager (similar to how uv pip is supposedly a drop-in replacement for pip https://github.com/astral-sh/uv)?

In theory, if we were less strict, we could have that drop in here and start working without needing pants changes... but, in practice, for this particular example, being strict sounds good: there's already 3 different styles of package manager, how will pants know that cool-new-package-manager should use pnpm vs. npm vs. yarn CLI? So yeah, exceptions plus dedicated pants support seems like the way forward 👍

Taking it one step further, there seems to be a lot of if self.package_manager == "<specific string>" calls here. Maybe this NodeJsProject type should validate the project_manager as an enum, on construction, to give errors eagerly and not require each separate method to handle it. (I don't think we should block this targeted bugfix on making that improvement, though.)

huonw

Thanks for this!

huonw · 2024-02-20T05:26:51Z

src/python/pants/backend/javascript/nodejs_project_test.py

+        }
+    )
+    projects = rule_runner.request(AllNodeJSProjects, [])
+    assert {project.immutable_install_args for project in projects} == {


Thanks for writing some tests for this. For this particular test, I think it'd be good to match each particular package manager to the expected args, a bit different to the other tests in this file.

In particular, because this is a set, I think the old code would pass this this test, as the LHS and RHS will be {("clean-install,), ("install", "--frozen-lockfile"), ("install", "--immutable")} in for both old and new (just the counts of will change, but that's not captured).

A different way to phrase it might be by parametrizing for each of the package mangers:

@pytest.mark.parametrize( ("package_manager", "expected_args"), [ (None, ("clean-install",)), ("[email protected]", ("install", "--frozen-lockfile")) ... ] ) def test_immutable_install_args_property(package_manager: None | str, expected_args: tuple[str, ...], rule_runner: RuleRunner) -> None: # construct one package and check its args specifically

What do you think?

Thats an awesome suggestion, thanks.
Changed to use parametrize: 5177a9b

huonw · 2024-02-20T05:31:46Z

src/python/pants/backend/javascript/nodejs_project.py

    def immutable_install_args(self) -> tuple[str, ...]:
        if self.package_manager == "npm":
            return ("clean-install",)
+        if self.package_manager == "yarn":
+            if nodesemver.satisfies(self.package_manager_version, "1.x"):
+                return ("install", "--frozen-lockfile")
+            return ("install", "--immutable")
        return ("install", "--frozen-lockfile")


I think there's abstractly a bit of tension here with being Pants being overly constraining.

For instance, what if someone publishes a pnpm-CLI-compatible cool-new-package-manager (similar to how uv pip is supposedly a drop-in replacement for pip https://github.com/astral-sh/uv)?

In theory, if we were less strict, we could have that drop in here and start working without needing pants changes... but, in practice, for this particular example, being strict sounds good: there's already 3 different styles of package manager, how will pants know that cool-new-package-manager should use pnpm vs. npm vs. yarn CLI? So yeah, exceptions plus dedicated pants support seems like the way forward 👍

Taking it one step further, there seems to be a lot of if self.package_manager == "<specific string>" calls here. Maybe this NodeJsProject type should validate the project_manager as an enum, on construction, to give errors eagerly and not require each separate method to handle it. (I don't think we should block this targeted bugfix on making that improvement, though.)

kaos

nice

huonw

Thank you!

kevinobruno and others added 2 commits February 13, 2024 16:34

fix: package yarn v2

0dfefa5

Merge branch 'main' into yarn-v2-immutable-install

6943a63

huonw added the category:bugfix Bug fixes for released features label Feb 14, 2024

fix: break line

86475c8

kaos reviewed Feb 14, 2024

View reviewed changes

refactor: raise unsupported package manager

3fe7c6e

kevinobruno force-pushed the yarn-v2-immutable-install branch from 3a732f8 to 3fe7c6e Compare February 19, 2024 14:50

Merge branch 'main' into yarn-v2-immutable-install

d51ff6b

huonw reviewed Feb 20, 2024

View reviewed changes

huonw requested a review from tobni February 20, 2024 05:34

kevinobruno added 2 commits February 21, 2024 12:02

test: use parametrize

5177a9b

Merge branch 'main' into yarn-v2-immutable-install

0223076

kevinobruno requested review from kaos and huonw February 21, 2024 15:27

kaos approved these changes Feb 21, 2024

View reviewed changes

huonw approved these changes Feb 21, 2024

View reviewed changes

huonw merged commit 5e27ae9 into pantsbuild:main Feb 21, 2024
24 checks passed

purajit mentioned this pull request Jun 26, 2024

Bump default terraform version to 1.9.0, add all known terraform versions inbetween #21110

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

javascript: package yarn v2 #20540

javascript: package yarn v2 #20540

kevinobruno commented Feb 13, 2024

kaos left a comment

kaos Feb 14, 2024

kevinobruno Feb 15, 2024

kaos Feb 16, 2024

kaos Feb 16, 2024

kevinobruno Feb 19, 2024 •

edited

Loading

huonw Feb 20, 2024

huonw left a comment

huonw Feb 20, 2024

kevinobruno Feb 21, 2024

huonw Feb 20, 2024

kaos left a comment

huonw left a comment

javascript: package yarn v2 #20540

javascript: package yarn v2 #20540

Conversation

kevinobruno commented Feb 13, 2024

kaos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinobruno Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huonw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaos left a comment

Choose a reason for hiding this comment

huonw left a comment

Choose a reason for hiding this comment

kevinobruno Feb 19, 2024 •

edited

Loading