Add `directory` key to package.json `repository` #1233

sambostock · 2020-01-12T05:33:41Z

Description

TL;DR

#1224 should have made this change instead:

 "name": "@shopify/{{name}}",
 "repository": {
   "type": "git",
   "url": "git+https://github.com/Shopify/quilt.git",
+  "directory": "packages/{{name}}",
 },

Details

This follows up on #1224, which updated the repository fields, in an effort to improve support for tools such as Dependabot to deep-link into the repo:

 "name": "@shopify/{{name}}",
-"repository": {
-  "type": "git",
-  "url": "git+https://github.com/Shopify/quilt.git"
-},
+"repository": "https://github.com/shopify/quilt/tree/master/packages/{{name}}",

While these links do work, they still did not improve integration with Dependabot.

After speaking to Dependabot support, and being referred to React's package.json repository entry, it looks like specifying the additional directory key is the answer:

 "name": "@shopify/{{name}}",
-"repository": "https://github.com/shopify/quilt/tree/master/packages/{{name}}",
+"repository": {
+  "type": "git",
+  "url": "git+https://github.com/Shopify/quilt.git",
+  "directory": "packages/{{name}}",
+},

This makes that change, fixes some other small inconsistencies, and adds tests along the way.

Type of change

Patch: Bug/ Documentation fix (non-breaking change which fixes an issue or adds documentation)

Checklist

I have added a changelog entry, prefixed by the type of change noted above

sambostock

There are a few outstanding discrepancies causing the tests for some packages to fail. I'm looking for feedback from reviewers on how to address them.

Once they've been addressed, I'll update the tests, remove the FIXME comments, and address any other linting errors.

test/consistent-package-json.test.ts

sambostock · 2020-01-12T05:40:01Z

test/consistent-package-json.test.ts

+    });
+
+    it('specifies the expected main', () => {
+      if(packageName === 'graphql-persisted') return; // FIXME: Address this in graphql-persisted


The graphql-persisted package does not specify an entry for main.

Yah, a main doesn't really make sense to me here. The package has two equally-important consumers (koa, apollo), so there isn't a good default import source.

Would it not still make sense to have a main index.js that consists of something like

export {apollo} from './apollo'; export {apollo} from './koa';

This would make it conform to the expected main entry point, while still offering the option to import the desired consumer directly. As a side benefit, it would also allow the types entry to be consistent.

test/consistent-package-json.test.ts

sambostock · 2020-01-12T05:42:34Z

test/consistent-package-json.test.ts

+    });
+
+    it('specifies the expected types', () => {
+      if(packageName === 'graphql-persisted') return; // FIXME


The graphql-persisted package fails to specify types.

Similar to the main conversation for this package, this seems fine to me. Consumers will directly import:

@shopify/graphql-persisted/apollo, which ships with apollo typedefs

@shopify/graphql-persisted/koa, which ships with koa typedefs

test/consistent-package-json.test.ts

GoodForOneFare

👍 A lot of bikeshedding from me about removing some cognitive load from test expectations, but everything looks sound to me. Thank you 💟

test/consistent-package-json.test.ts

GoodForOneFare · 2020-01-13T19:41:51Z

test/consistent-package-json.test.ts

+    });
+
+    it('specifies the expected main', () => {
+      if(packageName === 'graphql-persisted') return; // FIXME: Address this in graphql-persisted


Yah, a main doesn't really make sense to me here. The package has two equally-important consumers (koa, apollo), so there isn't a good default import source.

test/consistent-package-json.test.ts

GoodForOneFare · 2020-01-13T19:57:37Z

test/consistent-package-json.test.ts

+    });
+
+    it('specifies the expected types', () => {
+      if(packageName === 'graphql-persisted') return; // FIXME


Similar to the main conversation for this package, this seems fine to me. Consumers will directly import:

@shopify/graphql-persisted/apollo, which ships with apollo typedefs

@shopify/graphql-persisted/koa, which ships with koa typedefs

packages/web-worker/package.json

test/consistent-package-json.test.ts

GoodForOneFare · 2020-01-13T20:43:32Z

test/consistent-package-json.test.ts

+    });
+
+    it('specifies the MIT license', () => {
+      expect(packageJSON.license).toBe(packageJSONTemplate.license);


Suggested change

expect(packageJSON.license).toBe(packageJSONTemplate.license);

expect(packageJSON.license).toBe('MIT');

I had originally hardcoded things like this in this file, but then I started adding tests for the template as well. I realized that it made more sense to treat the template as the expected values, and ensure each package conforms to it.

If we hard code MIT here, it means that if we change the template down the road, the change will only be detected by the test suite after generating a new package and seeing it fail the suite. By coupling to the template, changing the license there would cause every package's license test to fail, which is likely what we want, since we'd probably want to update their licenses as well.

GoodForOneFare · 2020-01-13T20:47:08Z

test/consistent-package-json.test.ts

+    });
+
+    it('specifies Quilt deep-link homepage', () => {
+      const expectedHomepage = compile(packageJSONTemplate.homepage);


I know it's guaranteeing less, but I'd be happy enough with:

expect(packageJSON.homepage).toEndWith(`/${basename(packageJSONTemplatePath)}/README.md`)

I'm not sure what loosening this would gain us. I think the compile version is clearer, since it matches what is used in the other tests, and couples to the template as authoritative.

GoodForOneFare · 2020-01-13T20:49:26Z

test/consistent-package-json.test.ts

+    it('specifies a repository deep-linking into the Quilt monorepo', () => {
+      const expectedRepository = {
+        ...packageJSONTemplate.repository,
+        directory: compile(packageJSONTemplate.repository.directory),


Again, simplifying this to check for packages/${basename(packageJsonPath)} seems fine.

sambostock

Thanks for the review @GoodForOneFare! I've responded to a number of the comments.

Something that came up in a couple comments was the use of the compile helper, and the suggestion to decouple from the template. I feel that the template code and package specifications should match. Updating the template should probably require updating the packages to match. Decoupling from the template means packages can diverge, leading to inconsistencies, even if small.

That said, I agree that in some cases the use of compile is a little confusing. I considered deep-compiling the entire template for each package. This would allow usage like

expect(packageJSON.foo).toBe(expectedPackageJSON.foo);

I opted not to, as it would mean having to deeply compile the entire template for each package. If that's clearer though, I could switch it over to that.

It does occur to me that I could replace {{name}} in the raw JSON before parsing it, which would be simpler than deeply replacing {{name}} in the parsed object.

test/consistent-package-json.test.ts

sambostock · 2020-01-13T21:05:49Z

test/consistent-package-json.test.ts

+    });
+
+    it('specifies Quilt deep-link homepage', () => {
+      const expectedHomepage = compile(packageJSONTemplate.homepage);


I'm not sure what loosening this would gain us. I think the compile version is clearer, since it matches what is used in the other tests, and couples to the template as authoritative.

sambostock · 2020-01-13T21:10:05Z

test/consistent-package-json.test.ts

+    });
+
+    it('specifies the MIT license', () => {
+      expect(packageJSON.license).toBe(packageJSONTemplate.license);


I had originally hardcoded things like this in this file, but then I started adding tests for the template as well. I realized that it made more sense to treat the template as the expected values, and ensure each package conforms to it.

If we hard code MIT here, it means that if we change the template down the road, the change will only be detected by the test suite after generating a new package and seeing it fail the suite. By coupling to the template, changing the license there would cause every package's license test to fail, which is likely what we want, since we'd probably want to update their licenses as well.

sambostock · 2020-01-13T21:14:23Z

test/consistent-package-json.test.ts

+    });
+
+    it('specifies the expected main', () => {
+      if(packageName === 'graphql-persisted') return; // FIXME: Address this in graphql-persisted


Would it not still make sense to have a main index.js that consists of something like

export {apollo} from './apollo'; export {apollo} from './koa';

This would make it conform to the expected main entry point, while still offering the option to import the desired consumer directly. As a side benefit, it would also allow the types entry to be consistent.

test/consistent-package-json.test.ts

sambostock · 2020-01-13T21:21:17Z

test/consistent-package-json.test.ts

+      expect(packageJSON.types).toBe(packageJSONTemplate.types);
+    });
+
+    it('specifies a version', () => {


Fair. Should it be this file's responsibility to catch it before it gets to the publish stage though?

sambostock · 2020-01-13T22:21:14Z

@GoodForOneFare, I've added a commit in which I switch to pre-compiling the expected template for each package, instead of using compile on a per-value basis. It does look much cleaner, while keeping things based on the template, though at the cost of generating the expected contents each time (probably fine).

sambostock · 2020-01-13T23:42:48Z

I've addressed all exceptional packages and lint errors:

polyfills now specifies files: ["dist/*", "!tsconfig.tsbuildinfo"]
csrf-token-fetcher, dates, react-effect, react-google-analytics, and useful-types have had sideEffects: false added

graphql-persisted now exposes a main entrypoint which re-exports the existing entrypoints

import * as apollo from './apollo';
import * as koaMiddleware from './koa-middleware';

export {apollo, koaMiddleware};

@GoodForOneFare, should these changes have accompanying changelog entries? Should they be in a separate PR, or are they fine here?

packages/graphql-persisted/package.json

packages/graphql-persisted/src/index.ts

test/consistent-package-json.test.ts

GoodForOneFare · 2020-01-14T05:45:00Z

test/consistent-package-json.test.ts

+      expect(packageJSON.types).toBe(packageJSONTemplate.types);
+    });
+
+    it('specifies a version', () => {


It's a moot point because IMO this test will never catch anything 🤷🏻‍♂️ If someone doesn't use the generator:

Other 💩 will fail (like the sideEffects check) and they'll end up using the generator / copy+pasting from elsewhere

Worst case, they'll use yarn init, which also includes a version

I don't want to delay shipping the PR because of this, but I'd strongly prefer that tests for the sake of completion don't make it into the codebase.

test/consistent-package-json.test.ts

GoodForOneFare · 2020-01-14T06:23:25Z

RE changelog entries, I guess they have to be part of this PR. Otherwise, any merge gap between this and a changelog PR will result in someone else having to release everything.

sambostock · 2020-01-24T21:50:28Z

@GoodForOneFare, I've done the following:

Restored graphql-persisted to what it was, so it is now an exception to the main and types tests.
Added selected changelog entries:
- For the additions of "sideEffects": false across a couple packages
- For the change to published "files" in polyfills
Omitted changelog entries for "Shopify" capitalization, and the directory key, since I don't think consumers need to care.
Rebased into meaningful commits, so the diffs are trivial to inspect.

While I had originally agreed with your point on consolidating the tests into one test per key for all packages, I have switched it back to my original one test per key per package. I feel strongly that this is a better developer experience. You brought up two main points:

The end result is less noisy during successful runs

I did some testing, and as far as I can tell, the giant output only shows up if running this single test file, all on its own. When running a typical run, this file shows up just like any other suite (file name only):
```
...
PASS packages/react-i18n/src/test/e2e/server.test.tsx
PASS test/consistent-package-json.test.ts                <<<<
PASS packages/i18n/src/test/pseudotranslate.test.ts
...
```
I noticed, when looking at the CI output, the console.log, console.warn, and console.error output that makes it into the test output provides far more noise than this does. 😅
It's rare to touch >3 package.jsons in a PR, so the set of packages to sift through is small

This applies to PRs that change a package's package.json, but changes to the template, or which add a new field benefit greatly from knowing which package(s) failed. The consolidated output is effectively useless, since it just says there has been a failure and provides no info about where or how many occurrences there are. During my rebase, I had to redo some of my changes, and knowing where I needed to add sideEffects meant I didn't have to sift through 70+ package.json files.

Based on that, I think the benefits of granular failures far outweigh the cost.

GoodForOneFare

👍 You're a champ, @sambostock! Very much necessary, and very much appreciated 🙏

The existing test file did not match the testRegex, so it was not running. Changing the extension to .test.ts allows it to match the regex, and generating separate tests for each package allows for more granular failures.

sambostock added Type: Enhancement 📈 Enhancement to our codebase Area: dev experience labels Jan 12, 2020

sambostock requested a review from GoodForOneFare January 12, 2020 05:33

sambostock commented Jan 12, 2020

View reviewed changes

sambostock changed the title ~~Standardize package dot json~~ Add directory key to package.json repository Jan 12, 2020

sambostock force-pushed the standardize-package-dot-json branch from b88f11d to 44d5e8f Compare January 12, 2020 05:57

sambostock commented Jan 12, 2020

View reviewed changes

test/consistent-package-json.test.ts Outdated Show resolved Hide resolved

sambostock force-pushed the standardize-package-dot-json branch from 44d5e8f to c95189b Compare January 13, 2020 16:10

GoodForOneFare approved these changes Jan 13, 2020

View reviewed changes

sambostock commented Jan 13, 2020

View reviewed changes

GoodForOneFare suggested changes Jan 14, 2020

View reviewed changes

sambostock force-pushed the standardize-package-dot-json branch 3 times, most recently from eb24a36 to b85e704 Compare January 24, 2020 21:48

GoodForOneFare approved these changes Jan 27, 2020

View reviewed changes

sambostock added 7 commits January 27, 2020 13:32

Fix package name tests

1f26ed6

The existing test file did not match the testRegex, so it was not running. Changing the extension to .test.ts allows it to match the regex, and generating separate tests for each package allows for more granular failures.

Consistently capitalize "Shopify" in bugs url

dce89c9

Use directory subkey in package.json repository

1ca51c3

Ensure all packages consistently specify files

d78121e

Ensure consistent package homepage deep link

3bc10ed

Ensure all packages specify sideEffects

32979a6

Ensure remaining package.json values consistent

ee4541b

sambostock force-pushed the standardize-package-dot-json branch from b85e704 to ee4541b Compare January 27, 2020 18:35

sambostock merged commit bb88ea2 into master Jan 27, 2020

sambostock deleted the standardize-package-dot-json branch January 27, 2020 18:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `directory` key to package.json `repository` #1233

Add `directory` key to package.json `repository` #1233

sambostock commented Jan 12, 2020

sambostock left a comment

sambostock Jan 12, 2020

GoodForOneFare Jan 13, 2020

sambostock Jan 13, 2020

sambostock Jan 12, 2020

GoodForOneFare Jan 13, 2020

GoodForOneFare left a comment

GoodForOneFare Jan 13, 2020

GoodForOneFare Jan 13, 2020

GoodForOneFare Jan 13, 2020

sambostock Jan 13, 2020

GoodForOneFare Jan 13, 2020

sambostock Jan 13, 2020

GoodForOneFare Jan 13, 2020

sambostock left a comment

sambostock Jan 13, 2020

sambostock Jan 13, 2020

sambostock Jan 13, 2020

sambostock Jan 13, 2020

sambostock commented Jan 13, 2020

sambostock commented Jan 13, 2020

GoodForOneFare Jan 14, 2020

GoodForOneFare commented Jan 14, 2020

sambostock commented Jan 24, 2020

GoodForOneFare left a comment

	expect(packageJSON.license).toBe(packageJSONTemplate.license);
	expect(packageJSON.license).toBe('MIT');

Add directory key to package.json repository #1233

Add directory key to package.json repository #1233

Conversation

sambostock commented Jan 12, 2020

Description

TL;DR

Details

Type of change

Checklist

sambostock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GoodForOneFare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sambostock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sambostock commented Jan 13, 2020

sambostock commented Jan 13, 2020

Choose a reason for hiding this comment

GoodForOneFare commented Jan 14, 2020

sambostock commented Jan 24, 2020

GoodForOneFare left a comment

Choose a reason for hiding this comment

Add `directory` key to package.json `repository` #1233

Add `directory` key to package.json `repository` #1233