Match v6 lockfile behaviour in v9 around injected deps #1924
Conversation
- Adds `.git` to `.bazelignore` (editors see `.git` when inspecting test logs otherwise) - Fixed typo `file` -> `find` in `npm/private/npm_translate_lock_generate.bzl` - Documented params for `_new_package_info` in `npm/private/pnpm.bzl` - Documented and tested `_convert_pnpm_v6_v9_version_peer_dep` in `npm/private/pnpm.bzl` - `.strip` over `.(r|l)strip` in `npm/private/pnpm.bzl` - Added tests for v6 and v9 lockfiles covering current behaviour around injected deps (unsupported feature that is non-fatal when used in v6 and now v9) - Remove inlined JSON in favour of Starlark + `json.encode` in `npm/private/test/parse_pnpm_lock_tests.bzl` - Added test suite target for `//npm/private/test:test_parse_pnpm_lock`
|
| @@ -485,8 +508,20 @@ def _convert_v9_packages(packages, snapshots): | |||
| # package_data can have the resolved "version" for things like https:// deps | |||
| friendly_version = package_data["version"] if "version" in package_data else static_key[version_index + 1:] | |||
|
|
|||
| package_id = package_snapshot.get("id", None) | |||
There was a problem hiding this comment.
Do we have any tests that show the side-effects of this change? Can we see that in a diff?
| @@ -499,6 +534,11 @@ def _convert_v9_packages(packages, snapshots): | |||
| resolution = package_data.get("resolution"), | |||
| ) | |||
|
|
|||
| # Match v6 lockfile behaviour with local dependency keys | |||
| if "file:" in package_key: | |||
There was a problem hiding this comment.
This as well. I'd like to see this change along with a test demonstrating the side-effects...
|
Quick update as I've not been able to return to this as soon as I would have liked, and likely won't be able to until the internal project this change was made for is complete. As for the questions, I don't believe any of the existing tests demonstrate the issue. New tests cases in this PR do (partially, they don't show the final label but they do show the malformed data that leads generation of the bad label), however since this PR also aligns v9 with v6 that isn't obvious. Splitting the test changes into a separate PR and merging that first (with the erroneous outputs) would it a lot more obvious exactly what this PR changes. If this is acceptable, I'll be happy to implement the split once I'm back on this. |
|
Splitting the PR into multiple would be great and make this a lot easier to merge one thing at a time. The minor cleanup/refactoring/comments can probably be done in a few isolated PRs? Then the actual fixes + tests (which faile A673 d before the fix) in their own PRs. Also checkout #1989, does that overlap with your |
|
Tests (and other trivial changes) split to #2004 |
This PR makes a few tweaks to how package references are acquired and processed such that handling of injected dependencies (unsupported, but non-fatal with v6 lockfile) works as expected with the new v9 lockfile.
Additionally;
.gitto.bazelignore(editors see.gitwhen inspecting test logs otherwise)file->findinnpm/private/npm_translate_lock_generate.bzl_new_package_infoinnpm/private/pnpm.bzl_convert_pnpm_v6_v9_version_peer_depinnpm/private/pnpm.bzl.stripover.(r|l)stripinnpm/private/pnpm.bzljson.encodeinnpm/private/test/parse_pnpm_lock_tests.bzl//npm/private/test:test_parse_pnpm_lockChanges are visible to end-users: yes(-ish)
Test plan