Conversation
Jason3S
changed the title
Jason3S
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new Flatpack JSON V2 format aimed at producing more diff-friendly serialized output, including a string table for compact/deduped string storage and a configurable pretty stringifier.
Changes:
- Added Flatpack format selection (
V1/V2) and new V2 storage/string-table machinery. - Updated
stringifyFlatpackedto emit more diff-friendly, line-batched array formatting (optionally). - Expanded optimization logic to support V2 (string table + negative string refs) and added supporting utilities/tests (Trie, WeakCache, RefCounter).
Reviewed changes
Copilot reviewed 41 out of 55 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/flatpack-json/src/types.mts | Adds V2 headers/format option, string-table element types, and unpack annotations. |
| packages/flatpack-json/src/stringify.mts | Adds diff-friendly line formatting with options. |
| packages/flatpack-json/src/stringTable.mts | Introduces StringTable and StringTableBuilder. |
| packages/flatpack-json/src/stringTable.test.mts | Adds tests for string table behavior (currently contains duplicate test block). |
| packages/flatpack-json/src/storageV2.mts | Adds CompactStorageV2 using string-table-based string encoding and improved caching/dedupe. |
| packages/flatpack-json/src/storageV1.mts | Extracts legacy V1 compact storage into its own module. |
| packages/flatpack-json/src/storageV1.test.mts | Adds snapshot tests for V1 storage output and roundtrips. |
| packages/flatpack-json/src/storage.mts | Routes toJSON/stringify to V1 or V2 based on normalized options. |
| packages/flatpack-json/src/storage.test.mts | Extends tests for V1 vs V2 + adds v1→v2 conversion test (currently writes fixture output). |
| packages/flatpack-json/src/proxy.mts | Removes a stray console.log from proxy map updates. |
| packages/flatpack-json/src/optimizeFlatpacked.mts | Updates optimizer to account for string tables and negative string references. |
| packages/flatpack-json/src/WeakCache.mts | Adds mixed primitive/object key cache implementation. |
| packages/flatpack-json/src/WeakCache.test.mts | Adds tests for WeakCache behavior. |
| packages/flatpack-json/src/Trie.mts | Refactors trie implementation; introduces generic Trie and deprecates TrieOfStrings. |
| packages/flatpack-json/src/Trie.test.mts | Adds tests for new trie behavior. |
| packages/flatpack-json/src/RefCounter.mts | Adds reference counter utility. |
| packages/flatpack-json/src/RefCounter.test.mts | Adds tests for RefCounter. |
| packages/flatpack-json/src/RefElements.mts | Updates index typing from Index to FlatpackIndex. |
| packages/flatpack-json/src/Flatpack.mts | Simplifies FlatpackStore to delegate to FlatpackStoreV1. |
| packages/flatpack-json/src/FlatpackV1.mts | Adds FlatpackStoreV1 implementation (split from previous FlatpackStore). |
| packages/flatpack-json/src/FlatpackV1.test.mts | Updates tests to target FlatpackStoreV1. |
| packages/flatpack-json/src/FlatpackV2.mts | Adds FlatpackStoreV2 implementation. |
| packages/flatpack-json/src/FlatpackV2.test.mts | Adds tests for FlatpackStoreV2, including diff-generation snapshots. |
| packages/flatpack-json/src/CompactStorage.mts | Adds common CompactStorage abstraction. |
| packages/flatpack-json/src/snapshots/storageV2_fileList.json | Adds snapshot artifact for V2 file list output. |
| packages/flatpack-json/src/snapshots/storageV2_fileList.data.json | Adds snapshot artifact for V2 file list input data. |
| packages/flatpack-json/src/snapshots/FlatpackV2_fileList.data.json | Adds snapshot artifact for FlatpackV2 file list input data. |
| packages/flatpack-json/src/snapshots/storageV1.test.mts.snap | Adds Vitest snapshots for V1 storage tests. |
| .prettierignore | Ignores .npm*.json fixture outputs from formatting. |
Comment on lines
+40
to
+42
| // Note: WeakMap.has will always return false for primitive keys, | ||
| // so we can check both caches without first checking if it is an object. | ||
| return this.#primitiveCache.has(key) || this.#objectCache.has(key as object); |
| const data = fromJSON(JSON.parse(contentNpmV1)); | ||
| expect(fromJSON(toJSON(data, { format: 'V2', dedupe: true }))).toEqual(data); | ||
| const jsonStr = stringify(data, true, { optimize: true, format: 'V2', dedupe: true }); | ||
| await fs.writeFile(new URL('.npm-packages-info-v2.json', urlFixtures), jsonStr); |
Comment on lines
+398
to
+412
| private useFlatpackMetaData(data: UnpackMetaData | undefined): void { | ||
| if (!data || data.flatpack[0] !== dataHeaderV2_0) { | ||
| return; | ||
| } | ||
| this.unpackMetaData = data; | ||
| const flatpack: Flatpacked = [...data.flatpack]; | ||
| const st = flatpack[1]; | ||
| assert(isStringTableElement(st), 'Expected a string table element in the flatpack meta data'); | ||
| this.stringTable = new StringTableBuilder(st); | ||
| this.data = flatpack; | ||
| this.initFromFlatpackData(flatpack); | ||
| // Clear the referenced indexes since we don't want to treat them as referenced when we re-pack the data. | ||
| this.referenced.clear(); | ||
| this.softReset(); | ||
| } |
Comment on lines
+66
to
+82
| let added = 0; | ||
| let node: TrieNode<K, T> = this.root; | ||
| for (const k of key) { | ||
| let c = node.c; | ||
| if (!c) { | ||
| node.c = c = new Map(); | ||
| added = 1; | ||
| } | ||
| let n = c.get(k); | ||
| if (!n) { | ||
| c.set(k, (n = {})); | ||
| added = 1; | ||
| } | ||
| node = n; | ||
| } | ||
| node.d = data; | ||
| this.size += added; |
| } | ||
|
|
||
| get size(): number { | ||
| return this.stringTableElement.length; |
Comment on lines
+206
to
+212
| sortEntriesByRefCount(): Map<number, number> { | ||
| const mapEntryToOldIndex = new Map<BuilderEntry, number>(this.#entries.map((entry, index) => [entry, index])); | ||
|
|
||
| const entry0 = this.#entries[0]; | ||
| const sorted = this.#entries.sort((a, b) => | ||
| a === entry0 ? -1 : b === entry0 ? 1 : b.refCount - a.refCount || getOldIndex(a) - getOldIndex(b), | ||
| ); |
Comment on lines
+27
to
+46
| test('StringTableBuilder', () => { | ||
| const builder = new StringTableBuilder(); | ||
| const idx1 = builder.add('apple'); | ||
| const idx2 = builder.add('banana'); | ||
| const idx3 = builder.add('apple'); | ||
| expect(idx1).toBe(idx3); | ||
| expect(builder.get(idx1)).toBe('apple'); | ||
| expect(builder.get(idx2)).toBe('banana'); | ||
| const stringTableElement = builder.build(); | ||
| const stringTable = new StringTable(stringTableElement); | ||
| expect([...stringTable.entries()]).toEqual([ | ||
| [1, 'apple'], | ||
| [2, 'banana'], | ||
| ]); | ||
| expect([...stringTable.values()]).toEqual(['apple', 'banana']); | ||
|
|
||
| expect(builder.getRefCount(idx1)).toBe(2); | ||
| expect(builder.getRefCount(idx2)).toBe(1); | ||
| }); | ||
|
|
| optimize?: boolean; | ||
|
|
||
| /** | ||
| * The format of the output. If not specified, the latest format will be used. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.