Generate stable JSON from generate_spec_json.js #2843
Conversation
dconeybe
requested review from
Feiyang1,
hiranya911 and
hsubox76
as code owners
Binary Size ReportAffected SDKs
Test Logs |
| @@ -24,6 +24,8 @@ import { addEqualityMatcher } from '../../util/equality_matcher'; | |||
| import { SpecBuilder } from './spec_builder'; | |||
| import { SpecStep } from './spec_test_runner'; | |||
|
|
|||
| import * as stringify from 'json-stable-stringify'; | |||
There was a problem hiding this comment.
It's less important in tests but we should be getting in the habit of avoiding whole-module imports like this.
However, I'm not actually sure what the right way to do this import is. Does import stringify from 'json-stable-stringify' work?
There was a problem hiding this comment.
Unfortunately, no, not with our current tsconfig.json. I get the following error:
index.d.ts(25, 1): This module is declared with using 'export =', and can only be used with a default import when using the 'allowSyntheticDefaultImports' flag.
That being said, I'm not very familiar with TypeScript imports and all of the other variants that I tried did not work. Any other suggestions that I could try? In case it helps, here is what the index.d.ts looks like: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/json-stable-stringify/index.d.ts
There was a problem hiding this comment.
Ah, it looks like we're missing the esModuleInterop compiler setting.
@Feiyang1 is there any reason not to enable this repo-wide? We're already using it in our integration tests.
There was a problem hiding this comment.
We can enable it. I'm actually surprised to see that Typescript allows you to use stringify as a function directly. Thought it will give an error as it's not compliant with es6 modules spec.
| * ordering if one or both of the key are not overridden. | ||
| */ | ||
| function stringifyCmp(a: stringify.Element, b: stringify.Element): number { | ||
| // If the keys are equal, then avoid the costly computations below. |
There was a problem hiding this comment.
Performance doesn't matter here so I'd optimize for readability over anything else.
You can make this simpler by creating a function that determines the group number (use e.g. stringifyCustomOrdering.length for expected and stringifyCustomOrdering.length + 1 for "everything else").
Then you can just do:
const aGroup = stringifyGroup(a.key);
const bGroup = stringifyGroup(b.key);
if (aGroup === bGroup) {
return primitiveComparator(a.key, b.key); // from util/misc.ts
} else {
return primitiveComparator(aGroup, bGroup);
}There was a problem hiding this comment.
Done. Great suggestion.
| * defined in `stringifyCustomOrdering` is used, falling back to alphabetical | ||
| * ordering if one or both of the key are not overridden. | ||
| */ | ||
| function stringifyCmp(a: stringify.Element, b: stringify.Element): number { |
There was a problem hiding this comment.
Avoid abbreviations like this. Elsewhere we fully spell out "comparator".
package.json
Outdated
| @@ -128,5 +128,9 @@ | |||
| "hooks": { | |||
| "pre-push": "node tools/gitHooks/prepush.js" | |||
| } | |||
| }, | |||
| "dependencies": { | |||
There was a problem hiding this comment.
This should be a dev-dependency in packages/firestore.
|
@Feiyang1 Could you review for the files changed in the root directory (package.json and yarn.lock)? |
packages/firestore/package.json
Outdated
| @@ -49,6 +49,8 @@ | |||
| "@firebase/app-types": "0.x" | |||
| }, | |||
| "devDependencies": { | |||
| "@types/json-stable-stringify": "^1.0.32", | |||
The PR firebase/firebase-js-sdk#2843 caused stable JSON to be generated.
Currently, the JSON generated by
generate_spec_json.jsusesJSON.stringify(), which generates JSON with the keys ordered by the order in which they are set in the objects. As a result, code changes that alter the order in which the keys as added but have no other side effects cause different stringified JSON to be generated, resulting in unnecessarily-noisy diffs.This PR changes
generate_spec_json.jsto use the third-partyjson-stable-stringifylibrary: https://www.npmjs.com/package/json-stable-stringify. This library produces JSON that has a stable key ordering, resulting in diffs when and only when keys are added, removed, or have their values modified.I also implemented a custom comparator so that the generated JSON has a semblance of human readability, with the test "configuration" at the top and the "expected" blocks at the bottom.
I will submit follow-up PRs to the Android and iOS repositories with the new, stable JSON. These PRs will be very noisy but will allow for cleaner future diffs.