Add gitHead to package.json #543
Conversation
This is a field normally added automatically by npm publish. Closes microsoft#458
|
@sandersn before I solve the merge conflict, could you maybe take a look at this PR? |
| @@ -52,11 +52,12 @@ async function outputFilePath(pkg: AnyPackage, filename: string): Promise<string | |||
|
|
|||
| interface Dependencies { [name: string]: string; } | |||
|
|
|||
| async function createPackageJSON(typing: TypingsData, version: string, packages: AllPackages): Promise<string> { | |||
| async function createPackageJSON(typing: TypingsData, version: string, gitHead: string, packages: AllPackages): Promise<string> { | |||
| // Use the ordering of fields from https://docs.npmjs.com/files/package.json | |||
There was a problem hiding this comment.
the documentation doesn't mention gitHead, so I suspect this comment isn't true for gitHead. I'm not sure whether that matters though.
Any idea how to find out where npm's package.json puts gitHead? I tried looking at unpgk's version but it doesn't appear to be there either.
There was a problem hiding this comment.
You can find it in the registry API response: http://registry.npmjs.org/commander/latest
And npm should also write it out in node_modules when installing.
npm info commander gitHead also returns it (or npm info commander --json).
But the order of keys shouldn't really matter in an object.
It gets added by npm here: https://github.com/npm/read-package-json/blob/1b85cf87f5875774e7fbc617f606923d8cbda6a4/read-json.js#L345
| * Return the commit SHA1 of the master tip of DefinitelyTyped | ||
| */ | ||
| export async function resolveDefinitelyTypedMaster(githubAccessToken: string, fetcher: Fetcher): Promise<string> { | ||
| const masterRef = await queryGithub("repos/DefinitelyTyped/DefinitelyTyped/git/refs/heads/master", githubAccessToken, fetcher) as { object: { sha: string } }; |
There was a problem hiding this comment.
Is master the correct SHA to use here? In particular, it's possible that it may have moved between the time that this package was merged. Another package may have been merged in between.
I think it would be better if you use the latest sha just for the package's directory.
There was a problem hiding this comment.
It doesn't really matter - this SHA will be used to download the zip, so it is guaranteed that gitHead points to the exact revision that is being published. That is all that should matter.
It would be nice, but I wouldn't know where to get that commit from. Can you point to where?
There was a problem hiding this comment.
Here’s the code I used, although that was using octokit. The rest call shouldn’t be too different though.
DefinitelyTyped/types-publisher-watchdog@f647115#diff-168726dbe96b3ce427e7fedce31bb0bcL76
Look at the part I deleted starting with const oops
There was a problem hiding this comment.
Ah, so you would get the latest commit from the git log of that folder. I thought you were suggesting to use the commit ID from the triggering event, e.g. the commit hash of the merge commit of a PR that was merged which triggered the new publish
|
I also thought it would be easier for me to understand the change with a test — I added jest a little while ago. If you don’t have time for that I can try to add it on Monday. |
|
Here's the test I added. However, I can't get github API requests to work at all (eg from + async definitelyTypedMasterResolvesToCommit() {
+ expect(process.env["GH_API_TOKEN"]).toBeDefined();
+ const s = await resolveDefinitelyTypedMaster(process.env["GH_API_TOKEN"]!, new Fetcher());
+ expect(typeof s).toBe("string");
+ expect(s.length).toBe(40);
+ expect(/[^a-z0-9]/.test(s)).toBe(true);
+ }I'll push a commit myself if I can get github API requests working but I don't have any more time today to do so. |
This is a field normally added automatically by npm publish. It is extremely useful to map packages back to a revision in the git repository (especially when releases are not tagged). Sourcegraph uses it to provide cross-repository go-to-definition.
Closes #458