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

Rename the lockfile types #146

Open
KSXGitHub opened this issue Oct 9, 2023 · 7 comments
Open

Rename the lockfile types #146

KSXGitHub opened this issue Oct 9, 2023 · 7 comments

Comments

@KSXGitHub
Copy link
Contributor

KSXGitHub commented Oct 9, 2023

From #143 (comment)

@anonrig

PkgNameVerPeer is really ambiguous. I PackageNameVersionPeer also doesn't make sense as a struct name.

@KSXGitHub

PkgNameVerPeer vs PackageNameVersionPeer preference aside. Naming this was particularly difficult for me, because I had to follow the lockfile format as defined in pnpm.

In pnpm, there are peculiar anonymous string types with defined syntax. The pnpm repo doesn't give them a name, only use string and parse it later (which is error-prone).

In Rust, I made it parses at the same time as serde. Though the names of these data structure is not clear as their purposes are overlapped and reused. Perhaps @zkochan can help us find better names for these Pkg* types?

@zkochan

I am not sure. Dependency path could be a good name because this is basically the directory name inside the virtual store. But dep path is already used for this + the custom registry. Maybe ShortDependencyPath? Then the one currently called dependency path would become FullDependencyPath?

And by the way, a short dependency path can be only a registry-hosted package. githosted and other types of dependencies have a different format for the dependency path.

@KSXGitHub

Dependency path could be a good name because this is basically the directory name inside the virtual store.

It's not exactly the names in the virtual store. The names in the lockfile uses ( and ), but they were all replaced with _. Furthermore, the names in the lockfile contain / for scoped names. I originally name it DependencyPath because the spec calls the keys of the snapshot object "dependency path".

TBH, I don't think this is a good name. It doesn't refer to any actual path, and it fails to describe the purpose of the type as well as the components.

Concatenating the components of a syntax together may have been a temporary solution at the time, but now that I think about it, it's the best solution so far.

And by the way, a short dependency path can be only a registry-hosted package. githosted and other types of dependencies have a different format for the dependency path.

Does this mean that this "short dependency path" should be an enum and name+ver+peer is a variant in it?

@zkochan

It's not exactly the names in the virtual store. The names in the lockfile uses ( and ), but they were all replaced with _. Furthermore, the names in the lockfile contain / for scoped names.

Like you said, it is the directory name with some conversion to make it a valid directory name.

Concatenating the components of a syntax together may have been a temporary solution at the time, but now that I think about it, it's the best solution so far.

You mean naming it PkgNameVerPeers? But then you also need to add PatchHash to it because it also can contain a patch hash.

Does this mean that this "short dependency path" should be an enum and name+ver+peer is a variant in it?

no, because you separated the custom registry field from name+ver+peer and custom registry is only related to name+ver+peer, not other types of "dependency path"

Maybe it can be named "Dependency ID"? We currently use Package ID to name name+ver. So name+ver+peers would be Dependency ID. But it could also be PkgSnapshotID as the entries in the lockfile are called PackageSnapshot.

@zkochan
Copy link
Member

zkochan commented Oct 9, 2023

Other possible option: DependencyVariation (or DependencyVariant). I think I like this one most.

@KSXGitHub
Copy link
Contributor Author

@zkochan

no, because you separated the custom registry field from name+ver+peer and custom registry is only related to name+ver+peer, not other types of "dependency path"

Custom registry is at it again! I have yet to encountered a lockfile with custom registry so it's hard for me to formulate a correct schema and structure.

Maybe it can be named "Dependency ID"? We currently use Package ID to name name+ver. So name+ver+peers would be Dependency ID. But it could also be PkgSnapshotID as the entries in the lockfile are called PackageSnapshot.

I will need to think more about this, later.

@zkochan
Copy link
Member

zkochan commented Oct 9, 2023

Custom registry is at it again! I have yet to encountered a lockfile with custom registry so it's hard for me to formulate a correct schema and structure.

Maybe we don't need custom registry support at all. I couldn't reproduce a lockfile that has them and I can't find related tests. I think they are not needed.

@KSXGitHub
Copy link
Contributor Author

@zkochan Is there an issue or a PR that introduces custom registry as a package version specifier in the lockfile?

@zkochan
Copy link
Member

zkochan commented Oct 9, 2023

Forget about custom registry in dependency path. It is legacy. We'll remove it from pnpm v9.

@KSXGitHub
Copy link
Contributor Author

Forget about custom registry in dependency path. It is legacy. We'll remove it from pnpm v9.

Does it mean that dependency path (that is, the keys in the snapshot objects) would no longer start with / prefix?

@zkochan
Copy link
Member

zkochan commented Oct 10, 2023

No, I don't think that will change.

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

No branches or pull requests

2 participants