TeemuKoivisto / prosemirror-dev-toolkit

Injectable developer tools for ProseMirror rich-text editors implemented in Svelte and TypeScript.
https://teemukoivisto.github.io/prosemirror-dev-toolkit/
MIT License
116 stars 6 forks source link

fix: don't call applyTransaction before dispatch #15

Closed ocavue closed 2 years ago

ocavue commented 2 years ago

Previously, prosemirror-dev-toolkit will call state.applyTransaction before call oldDispatchFn (if it exists). Since oldDispatchFn itself will most likely also trigger state.applyTransaction, we will trigger applyTransaction twice for the same transaction. This causes some issues because we have a plugin that assumes it can't see a transaction twice.

This PR fixes the issue above by creating a plugin that will record all transactions during a dispatch. We will extract recorded transactions later after dispatch and pass them to HistoryEntry. By doing that, we can avoid calling applyTransaction twice. This is similar to the official prosemirror-history plugin.

I can't directly pass this plugin to view by using view.someProps because it doesn't accept a plugin that has state. That's why I have to inject the plugin into view.state before dispatch.

TeemuKoivisto commented 2 years ago

Oh damn. I have been intending to fix this annoying bug but didn't realize it was in the toolkit. Thanks a ton making the fix! My only gripe is having to add prosemirror-state as a dependency with the Plugin now being directly imported.

So I tried another approach in https://github.com/TeemuKoivisto/prosemirror-dev-toolkit/pull/16 It's very ugly but it seems to work. Incase that one doesn't cut it, I'll merge your approach and just add prosemirror-state.

ocavue commented 2 years ago

16 works well in my case. Awesome! Will close my PR.


My only gripe is having to add prosemirror-state as a dependency.

I don't think it's a big deal to include prosemirror dependencies, as end-users can use yarn-deduplicate or similar features if they are using other package managers to resolve duplicate dependencies.

TeemuKoivisto commented 2 years ago

👍 Agree on that, shouldn't be a big hurdle. But it always feels bad to see the bundle size sticker go up 😃

ocavue commented 2 years ago

Well, we can move prosemirror to peerDependencies so that the bundle size won't go up. Anyone who uses toolkit will already get prosemirror installed anyway.

TeemuKoivisto commented 2 years ago

Hmm didn't consider that. Also there's the minified bundle which includes all the dependencies that I use to inject the tools which is quite brittle. Anyway, let's see how this evolves.