From 68a94b62f297ba73e231eee906823a075bf22be2 Mon Sep 17 00:00:00 2001 From: Shane Friedman Date: Wed, 13 Dec 2023 10:29:28 -0500 Subject: [PATCH 1/4] Map positions forward to maintain key stability Rather than attempting to map the new node positions backward to determine what the key used to be for that node, start with the previous positions and map them forward, taking care to skip any positions that are deleted by the transaction, and adding any new nodes that didn't exist in the old doc. This improves stability and correctness in more complex cases, such as splitting or wrapping nodes, and nested deletions. --- .vscode/settings.json | 5 +++++ .yarn/versions/1f4b945b.yml | 2 ++ src/plugins/__tests__/react.test.ts | 4 ++-- src/plugins/react.ts | 23 +++++++++-------------- 4 files changed, 18 insertions(+), 16 deletions(-) create mode 100644 .yarn/versions/1f4b945b.yml diff --git a/.vscode/settings.json b/.vscode/settings.json index 8939aae4..176af1c7 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -25,5 +25,10 @@ }, "[yaml]": { "editor.defaultFormatter": "esbenp.prettier-vscode" + }, + "[javascript][typescript]": { + "editor.codeActionsOnSave": { + "source.fixAll": "explicit" + } } } diff --git a/.yarn/versions/1f4b945b.yml b/.yarn/versions/1f4b945b.yml new file mode 100644 index 00000000..90948e78 --- /dev/null +++ b/.yarn/versions/1f4b945b.yml @@ -0,0 +1,2 @@ +releases: + "@nytimes/react-prosemirror": patch diff --git a/src/plugins/__tests__/react.test.ts b/src/plugins/__tests__/react.test.ts index 617ab433..1bd3ad82 100644 --- a/src/plugins/__tests__/react.test.ts +++ b/src/plugins/__tests__/react.test.ts @@ -46,8 +46,8 @@ describe("reactNodeViewPlugin", () => { ); const nextPluginState = reactPluginKey.getState(nextEditorState)!; - expect(Array.from(initialPluginState.keyToPos.keys())).toEqual( - Array.from(nextPluginState.keyToPos.keys()) + expect(Array.from(nextPluginState.keyToPos.keys())).toEqual( + Array.from(initialPluginState.keyToPos.keys()) ); }); diff --git a/src/plugins/react.ts b/src/plugins/react.ts index e0a5300e..402a2e0f 100644 --- a/src/plugins/react.ts +++ b/src/plugins/react.ts @@ -8,12 +8,6 @@ export const ROOT_NODE_KEY = Symbol("@nytimes/react-prosemirror/root-node-key"); export type NodeKey = string | typeof ROOT_NODE_KEY; -/** - * Identifies a node view constructor as having been created - * by @nytimes/react-prosemirror - */ -export const REACT_NODE_VIEW = Symbol("react node view"); - export function createNodeKey() { return Math.floor(Math.random() * 0xffffff).toString(16); } @@ -68,19 +62,20 @@ export function react() { posToKey: new Map(), keyToPos: new Map(), }; - const nextKeys = new Set(); + for (const [pos, key] of value.posToKey.entries()) { + const { pos: newPos, deleted } = tr.mapping.mapResult(pos); + if (deleted) continue; + + next.posToKey.set(newPos, key); + next.keyToPos.set(key, newPos); + } newState.doc.descendants((node, pos) => { if (node.isText) return false; + if (next.posToKey.has(pos)) return true; - const prevPos = tr.mapping.invert().map(pos); - const prevKey = value.posToKey.get(prevPos) ?? createNodeKey(); - // If this transaction adds a new node, there will be multiple - // nodes that map back to the same initial position. In this case, - // create new keys for new nodes. - const key = nextKeys.has(prevKey) ? createNodeKey() : prevKey; + const key = createNodeKey(); next.posToKey.set(pos, key); next.keyToPos.set(key, pos); - nextKeys.add(key); return true; }); return next; From f5c644a4d29910095729d3b25e3300f990ab7f52 Mon Sep 17 00:00:00 2001 From: Shane Friedman Date: Wed, 31 Jan 2024 15:23:28 -0500 Subject: [PATCH 2/4] Add tests for splitting and wrapping nodes --- src/plugins/__tests__/react.test.ts | 73 ++++++++++++++++++++++++++++- 1 file changed, 71 insertions(+), 2 deletions(-) diff --git a/src/plugins/__tests__/react.test.ts b/src/plugins/__tests__/react.test.ts index 1bd3ad82..9af55085 100644 --- a/src/plugins/__tests__/react.test.ts +++ b/src/plugins/__tests__/react.test.ts @@ -1,6 +1,7 @@ /* eslint-disable @typescript-eslint/no-non-null-assertion */ import { Schema } from "prosemirror-model"; import { EditorState } from "prosemirror-state"; +import { findWrapping } from "prosemirror-transform"; import { react, reactPluginKey } from "../react.js"; @@ -9,7 +10,7 @@ const schema = new Schema({ doc: { content: "block+" }, paragraph: { group: "block", content: "inline*" }, list: { group: "block", content: "list_item+" }, - list_item: { content: "inline*" }, + list_item: { content: "paragraph+" }, text: { group: "inline" }, }, }); @@ -69,10 +70,78 @@ describe("reactNodeViewPlugin", () => { const nextPluginState = reactPluginKey.getState(nextEditorState)!; // Adds new keys for new nodes - expect(nextPluginState.keyToPos.size).toBe(5); + expect(nextPluginState.keyToPos.size).toBe(6); // Maintains keys for previous nodes that are still there Array.from(initialPluginState.keyToPos.keys()).forEach((key) => { expect(Array.from(nextPluginState.keyToPos.keys())).toContain(key); }); }); + + it("should maintain key stability when splitting a node", () => { + const initialEditorState = EditorState.create({ + doc: schema.topNodeType.create(null, [ + schema.nodes.list.create(null, [ + schema.nodes.list_item.create(null, [ + schema.nodes.paragraph.create(null, [schema.text("first")]), + ]), + ]), + ]), + plugins: [react()], + }); + + const initialPluginState = reactPluginKey.getState(initialEditorState)!; + + const nextEditorState = initialEditorState.apply( + initialEditorState.tr.insert( + 1, + schema.nodes.list_item.create(null, [ + schema.nodes.paragraph.create(null, [schema.text("second")]), + ])! + ) + ); + const nextPluginState = reactPluginKey.getState(nextEditorState)!; + + // The new list item was inserted before the original one, + // pushing it further into the document. The original list + // item should keep its original key, and the new list item + // should be assigned a new one + expect(nextPluginState.posToKey.get(11)).toBe( + initialPluginState.posToKey.get(1) + ); + expect(nextPluginState.posToKey.get(1)).not.toBe( + initialPluginState.posToKey.get(1) + ); + }); + + it("should maintain key stability when wrapping a node", () => { + const initialEditorState = EditorState.create({ + doc: schema.topNodeType.create(null, [ + schema.nodes.paragraph.create(null, [schema.text("content")]), + ]), + plugins: [react()], + }); + + const initialPluginState = reactPluginKey.getState(initialEditorState)!; + const start = 1; + const end = 9; + const tr = initialEditorState.tr.delete(start, end); + const $start = tr.doc.resolve(start); + + const range = $start.blockRange()!; + const wrapping = range && findWrapping(range, schema.nodes.list, null)!; + tr.wrap(range, wrapping); + const nextEditorState = initialEditorState.apply(tr); + const nextPluginState = reactPluginKey.getState(nextEditorState)!; + + // The new list and list item nodes were wrapped around the + // paragraph, pushing it further into the document. The paragraph + // should keep its original key, and the new nodes + // should be assigned a new one + expect(nextPluginState.posToKey.get(2)).toBe( + initialPluginState.posToKey.get(0) + ); + expect(nextPluginState.posToKey.get(0)).not.toBe( + initialPluginState.posToKey.get(0) + ); + }); }); From dfbcb38c7f4abc201f77d8cd532504aee551f4d3 Mon Sep 17 00:00:00 2001 From: Shane Friedman Date: Wed, 31 Jan 2024 15:24:31 -0500 Subject: [PATCH 3/4] Remove fix-on-save settings from project settings --- .vscode/settings.json | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index 176af1c7..36e00333 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -7,28 +7,16 @@ "typescript.enablePromptUseWorkspaceTsdk": true, "prettier.prettierPath": ".yarn/sdks/prettier/index.js", "eslint.nodePath": ".yarn/sdks", - "editor.formatOnSave": true, "[javascript]": { - "editor.defaultFormatter": "esbenp.prettier-vscode", - "editor.codeActionsOnSave": { - "source.fixAll": true - } + "editor.defaultFormatter": "esbenp.prettier-vscode" }, "[typescript]": { - "editor.defaultFormatter": "esbenp.prettier-vscode", - "editor.codeActionsOnSave": { - "source.fixAll": true - } + "editor.defaultFormatter": "esbenp.prettier-vscode" }, "[markdown]": { "editor.defaultFormatter": "esbenp.prettier-vscode" }, "[yaml]": { "editor.defaultFormatter": "esbenp.prettier-vscode" - }, - "[javascript][typescript]": { - "editor.codeActionsOnSave": { - "source.fixAll": "explicit" - } } } From 3aeab68527ce5eea1dcb1317d739a712bc9254e8 Mon Sep 17 00:00:00 2001 From: Shane Friedman Date: Wed, 31 Jan 2024 15:25:57 -0500 Subject: [PATCH 4/4] Add prosemirror-transform as dev dep for testing --- package.json | 1 + yarn.lock | 10 ++++++++++ 2 files changed, 11 insertions(+) diff --git a/package.json b/package.json index 0999ec43..6864463b 100644 --- a/package.json +++ b/package.json @@ -71,6 +71,7 @@ "prosemirror-model": "^1.18.3", "prosemirror-schema-list": "^1.2.2", "prosemirror-state": "^1.4.2", + "prosemirror-transform": "^1.8.0", "prosemirror-view": "^1.29.1", "react": "^18.2.0", "react-dom": "^18.2.0", diff --git a/yarn.lock b/yarn.lock index 03d8d264..d6143c7d 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1251,6 +1251,7 @@ __metadata: prosemirror-model: ^1.18.3 prosemirror-schema-list: ^1.2.2 prosemirror-state: ^1.4.2 + prosemirror-transform: ^1.8.0 prosemirror-view: ^1.29.1 react: ^18.2.0 react-dom: ^18.2.0 @@ -6991,6 +6992,15 @@ __metadata: languageName: node linkType: hard +"prosemirror-transform@npm:^1.8.0": + version: 1.8.0 + resolution: "prosemirror-transform@npm:1.8.0" + dependencies: + prosemirror-model: ^1.0.0 + checksum: 6d16ca4f954ad7b040a4adbb5ddfa8c8ad14b0514f15e1ecfd5e32f08eb3f8696492975b9e599b4776e991ab76df114166dcf6ec7b966a67b02b2069a28415f1 + languageName: node + linkType: hard + "prosemirror-view@npm:^1.27.0, prosemirror-view@npm:^1.29.1": version: 1.30.1 resolution: "prosemirror-view@npm:1.30.1"