-
Notifications
You must be signed in to change notification settings - Fork 80
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
Registry Draft v2 #76
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is starting to look really nice ❤️
- the upstream location for the sources of the package | ||
- published versions and the SHA256 for their tarball as computed by [our CI](#Adding-a-new-package). | ||
Note: these are going to be sorted in ascending order according to [SemVer](https://semver.org) - when in | ||
doubt the sorting provided by [the `semver` package on NPM](https://www.npmjs.com/package/semver#comparison) is correct. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's a need to refer to the semver
package on npm, is there? The spec tells you how to sort versions unambiguously in all cases except for when the versions have the same major, minor, patch, and prerelease data, but their build metadata differs. However, I think this case should not come up, as the registry should not accept a package submission whose version is the same as an already-uploaded version in all respects except for the build metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The keyword here is "when in doubt". The bottom line here is that we go by the standard, but in practice our CI will use the NPM package (which is the main implementation of the standard anyways), so I thought it was useful to refer to that here so that in case of inconsistencies it would be easy to verify if the problem is in the standard, in this implementation or in other implementations.
|
||
See [the `Package` type definition](./v1/Package.dhall) for an up to date representation. | ||
This means that the only changes allowed to the schema are: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to "deprecate" a field by replacing it with a new one, with tools using the new one but falling back to the old one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could theoretically work, but the reason why we have to leave old fields in is so that old clients will be able to read them (as they cannot rely on new fields). In this sense we'll never be able to stop writing data to the old fields, so I guess the answer to your question is "not really". Makes sense?
@@ -103,321 +129,316 @@ A "compilation target". | |||
Every target can have its own dependencies, source globs, etc. | |||
By convention a package needs to have at least one target called `lib`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this part just enforced by CI? Or is there any reason to make TargetType
either a) Lib
(with an enforced "src/**/*.purs" source) or b) some other target?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I envisioned this as being just a CI check, but we could do something fancier as well here.
Right now the Dhall types are fairly basic, because there are some constraints on their values, which need to:
- have a decent JSON representation
- make it easy to guarantee forwards-compatibility
On the whole 👍! |
@hdgarrood @thomashoneyman I merged this and opened issues for the review comments where we figured out action points or where I felt there's the need for more discussion, feel free to open more issues if you feel like I didn't address some concern or if I missed some other actionable item. Thanks everyone for the thoughtful reviews! 🙂 |
This pull request introduces a major revision of the "new PureScript Registry draft" that we currently have in the README of the
master
branch.I'll call that "Draft v1", while this should be considered the "Draft v2".
Once this work graduates from the "draft" status (eventually after more revisions of this draft), it will be called "Registry v1" - this is important to note because we'll tie the version number to the breaking changes to the Dhall types of the Registry, currently in the
v1
folder.How to review this Pull Request
Before I get into the explanation of what has changed and how, I'd like to detail how I'd like the lifetime of this pull request to look like, and what I'd suggest the review workflow to be:
Since 90% of the work here consists of rewriting the spec in the README, I'd strongly suggest just having a read at the new draft
master
, as I'd find highly undesirable to be stuck with an outdated "draft v1" in there and the updated work only shown in this PR.However I believe it's useful to have this PR open so we have time to catch obvious oversights - so I'll wait for 3 days, and then merge to
master
Notable changes
I'll assume here that you're familiar with the current design, and I'll summarize here a list of things that have changed since then:
purs.json
registry-index
repository, for easier bulk-access.Metadata
file for every package, to be stored in this repo and to serve as permanent storage for data needed for registry operations. This includes storing the SHA256 of the package tarballs.nativeDependencies
has been made, and subsequently abandoned. See Backend specific dependencies #20 for context, and this commit for the (removed) code that was implementing this.Some of these changes have been discussed in issues in this repo, so I am not listing all the reasons for all the changes above (after all it's supposed to be a summary), so feel free to ask for more context and I'll do my best to link to relevant places and otherwise detail that.
Where do we go from here
You can find more details for the implementation plan in the relevant section of the draft.
Quoting from "what is happening now":
(1) and (3) are mostly this PR, (2) is happening but it's blocked by (1).
To summarize: we need to commit to the first version of the
Manifest
Schema before we move forward with the rest of this.