refactor: order generated packages in npm_translate_lock

[jbedard]

To reduce make the generated bzl more deterministic.

Changes are visible to end-users: no

Test plan

• Covered by existing test cases

[aspect-workflows]

Test

2 test targets passed

Targets

//npm/private:_test_gendocs_0_0 [k8-fastbuild] 31ms
//npm/private:_test_gendocs_0_1 [k8-fastbuild] 37ms

Total test execution time was 68ms. 223 tests (99.1%) were fully cached saving 33s.

---

Test

e2e/bzlmod

All tests were cache hits

5 tests (100.0%) were fully cached saving 502ms.

---

Test

e2e/gyp_no_install_script

All tests were cache hits

2 tests (100.0%) were fully cached saving 151ms.

---

Test

e2e/js_image_oci

All tests were cache hits

1 test (100.0%) was fully cached saving 2s.

---

Test

e2e/npm_link_package

All tests were cache hits

3 tests (100.0%) were fully cached saving 448ms.

---

Test

e2e/npm_link_package-esm

All tests were cache hits

3 tests (100.0%) were fully cached saving 533ms.

---

Test

e2e/npm_translate_lock

All tests were cache hits

1 test (100.0%) was fully cached saving 30ms.

---

Test

e2e/npm_translate_lock_empty

All tests were cache hits

1 test (100.0%) was fully cached saving 30ms.

---

Test

e2e/npm_translate_lock_multi

All tests were cache hits

2 tests (100.0%) were fully cached saving 268ms.

---

Test

e2e/npm_translate_lock_partial_clone

All tests were cache hits

1 test (100.0%) was fully cached saving 31ms.

---

Test

e2e/npm_translate_lock_replace_packages

All tests were cache hits

3 tests (100.0%) were fully cached saving 603ms.

---

Test

e2e/npm_translate_lock_subdir_patch

All tests were cache hits

1 test (100.0%) was fully cached saving 98ms.

---

Test

e2e/npm_translate_package_lock

All tests were cache hits

1 test (100.0%) was fully cached saving 31ms.

---

Test

e2e/npm_translate_yarn_lock

All tests were cache hits

1 test (100.0%) was fully cached saving 31ms.

---

Test

e2e/package_json_module

All tests were cache hits

1 test (100.0%) was fully cached saving 253ms.

---

Test

e2e/pnpm_lockfiles

4 test targets passed

Targets

//v54:repos_0_test [k8-fastbuild]              38ms
//v60:repos_0_test [k8-fastbuild]              31ms
//v61:repos_0_test [k8-fastbuild]              35ms
//v90:repos_0_test [k8-fastbuild]              36ms

Total test execution time was 140ms. 38 tests (90.5%) were fully cached saving 2s.

---

Test

e2e/pnpm_workspace

All tests were cache hits

10 tests (100.0%) were fully cached saving 2s.

---

Test

e2e/pnpm_workspace_rerooted

All tests were cache hits

12 tests (100.0%) were fully cached saving 2s.

---

Test

e2e/repo_mapping

All tests were cache hits

3 tests (100.0%) were fully cached saving 404ms.

---

Test

e2e/rules_foo

All tests were cache hits

2 tests (100.0%) were fully cached saving 187ms.

---

Test

e2e/runfiles

All tests were cache hits

1 test (100.0%) was fully cached saving 173ms.

---

Test

e2e/vendored_node

All tests were cache hits

1 test (100.0%) was fully cached saving 99ms.

---

Buildifier      Format

[dzbarsky]

starlark spec says "Iteration yields the dictionary's keys in the order in which they were inserted; updating the value associated with an existing key does not affect the iteration order." so not sure this is necessary?

[jbedard]

This did change the generated code, however maybe the importers should be sorted elsewhere? Or maybe it is deterministic (lockfile order) and there is no reason for this one. I might revert this line...

[dzbarsky]

i'm not sure of the full flow so its possible the non-determinism (was it observed or theoretical?) came from earlier. see comment below, .keys() and iteration should be deterministic

[jbedard]

I'm mainly trying to ensure that refactoring all the pnpm logic doesn't unnecessary change the order of these fields. Maybe it's my other branch breaking the deterministic order and this isn't the root cause of it so I'm going to convert this to a draft for now...

[jbedard]

I think you're right that we can depend on the insertion order. Most of the things that changed order in #2177 are due to other reasons so I'll close this for now.