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.

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

feat: terse lockfile #26

Merged
merged 26 commits into from
Aug 15, 2024
Merged

feat: terse lockfile #26

merged 26 commits into from
Aug 15, 2024

Conversation

dsherret
Copy link
Member

@dsherret dsherret commented May 31, 2024

Closes #19

@zebreus
Copy link

zebreus commented Jul 23, 2024

This PR seems to implement lockfile version 4 proposed in denoland/deno#23213. I want to experiment with some changes to the lockfile format to make it possible to build reproducible vendored dependencies from the lockfile only. I think it would be easier for me, if we can get this PR merged, so I can work from there.

What is the state of this PR / are there still some features missing?

@dsherret
Copy link
Member Author

What is the state of this PR / are there still some features missing?

It's basically good to go, but I need to get a final thumbs up from the team about the changes. I was busy working on workspaces the past month, but I'm planning to get this one merged within the next couple of weeks (and when it lands, it will only be behind the DENO_FUTURE=1 env var for the time being).

It seems that #30 is mostly refactoring changes? It might be better to land those separately.

@zebreus
Copy link

zebreus commented Jul 27, 2024

Nice to hear that lockfile v4 is actually on the roadmap.

#30 is a mostly fixing up and finishig this PR. I think at 8ab3681 most of the problems in this PR are fixed and I switched to refactoring. If you want to I can try to split it up into one PR for fixes and one for refactoring.

How does DENO_FUTURE work? I assume that the expected behaviour is that it is backwards-compatible with old lockfile versions. However while #30 is able to read all lockfiles versions when writing modified lockfiles it will always output v4 lockfiles. I am planning to add back the functionality to write v3 lockfiles in the coming days, based on your high performance v4 printer and not on serde.

@dsherret dsherret marked this pull request as ready for review August 14, 2024 17:03
@dsherret
Copy link
Member Author

dsherret commented Aug 14, 2024

How does DENO_FUTURE work? I assume that the expected behaviour is that it is backwards-compatible with old lockfile versions. However while #30 is able to read all lockfiles versions when writing modified lockfiles it will always output v4 lockfiles.

DENO_FUTURE=1 is an environment variable that can be set to test upcomming features. I don't want everything to upgrade to v4 because we want to try this out for a bit first before doing that. This PR errors if loading a v4 lockfile without that flag.

I am planning to add back the functionality to write v3 lockfiles in the coming days, based on your high performance v4 printer and not on serde.

I wouldn't recommend working on that as outputting v3 lockfiles will be removed once this is stabilized.

src/lib.rs Show resolved Hide resolved
Value::String(v) => Some(v),
_ => None,
})
.unwrap_or_else(|| "3".to_string()),
Copy link
Member

Choose a reason for hiding this comment

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

Is this on purpose to default to version 3 instead of 4?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. This is only on with DENO_FUTURE for now

Comment on lines +150 to +155
output.push_str(" \"");
output.push_str(key);
output.push_str("\": {\n");
output.push_str(" \"integrity\": \"");
output.push_str(&value.integrity);
output.push('"');
Copy link
Member

Choose a reason for hiding this comment

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

Is it really better to hand write JSON in this file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure. I wanted to avoid clones and haven't meadured the performance of this yet. I'm going to rewrite this once dropping v3 I think.

@dsherret dsherret merged commit 2b1491a into denoland:main Aug 15, 2024
1 check passed
@dsherret dsherret deleted the terse_lockfile branch August 15, 2024 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reduce verbosity when a single version changes
3 participants