[BREAKING/PROPOSAL] Generate unique string ids - #13
Conversation
| generateId?: () => string; | ||
| } | ||
|
|
||
| function defaultGenerateId(): string { |
There was a problem hiding this comment.
This default function is pretty silly -- we don't use it ourselves, and I didn't want to add a dependency to crypto.randomUUID for this. Would probably need a better approach (or if we make this work with both number and string ids), could revert back to the old numbering id system.
There was a problem hiding this comment.
In react-prosemirror we also just use Math.random(): https://github.com/handlewithcarecollective/react-prosemirror/blob/main/src/plugins/reactKeys.ts#L5C15-L5C71. I'd be fine with using that same approach here, honestly — it's probably enough!
| export const suggestChangesKey = new PluginKey<{ enabled: boolean }>( | ||
| export interface SuggestChangesPluginState { | ||
| enabled: boolean; | ||
| generateId: () => string; |
There was a problem hiding this comment.
We pass our id generator (@neuledge/id tagged id generator) here
There was a problem hiding this comment.
Rather than storing this in the state, since it's only ever used in the withSuggestChanges wrapper, maybe it should just be an optional argument to that function?
| generateId?: () => string; | ||
| } | ||
|
|
||
| function defaultGenerateId(): string { |
There was a problem hiding this comment.
In react-prosemirror we also just use Math.random(): https://github.com/handlewithcarecollective/react-prosemirror/blob/main/src/plugins/reactKeys.ts#L5C15-L5C71. I'd be fine with using that same approach here, honestly — it's probably enough!
| export const suggestChangesKey = new PluginKey<{ enabled: boolean }>( | ||
| export interface SuggestChangesPluginState { | ||
| enabled: boolean; | ||
| generateId: () => string; |
There was a problem hiding this comment.
Rather than storing this in the state, since it's only ever used in the withSuggestChanges wrapper, maybe it should just be an optional argument to that function?
| excludes: "insertion modification deletion", | ||
| attrs: { | ||
| id: { validate: "number" }, | ||
| id: { validate: "string" }, |
There was a problem hiding this comment.
... hm. It does stink that this is a breaking schema change — there's not really any good way to migrate from the previous implementation to this one, if you have any documents persisted with these marks. I'll think about it a bit more, there may be nothing to do about it (this is a challenge with ProseMirror schemas more generally)
|
I think this is a reasonable proposal! Happy to chat about it more if you have thoughts — it would be great to think about whether it was possible to avoid a breaking change, but this is a pre-v1 project, so if we're going to need a breaking change anyway, now's probably the time! |
@smoores-dev I think the Attribute spec can define the validator as a function, so we could make it accept strings or numbers. The type of the id could be either I haven't tried this, but in theory it should work just fine. New values would still be a breaking change for old schemas, but new schemas would support old ids and new ids. The backwards compatibility can be handled in userland by doing a staged migration -- upgrading the library first, and changing the Happy to make these changes if that sounds acceptable. In our application case we don't need this backwards compatibility, so I won't add the complexity to our fork unless you're happy to upstream it. |
|
Right, good call! That sounds great to me — if you're open to making those changes, I would be happy to accept them! |
|
@jevakallio are you still interested in taking this over the finish line? |
|
Yeah, absolutely. I have had to pivot my energies to other projects in the last month, but I'd love to be able to upstream this change (and other improvements we have made to the library) Let me try to find some time to round up these changes and open PRs. Apologies for leaving you hanging on this -- will aim to devote more energy towards it in the coming week/weeks. |
|
Sounds great, I really appreciate it! |
1086e65 to
d00c99c
Compare
|
Hey @smoores-dev I made the changes ( @jevakallio is away right now ), this is now backwards compatible. Now the id type is We'll have quite a few fixes & refactors coming soon, I'll open PRs! |
smoores-dev
left a comment
smoores-dev
left a comment
There was a problem hiding this comment.
This looks awesome. One last clarification question and then I think this is good to go!
| if (generateId && typeof suggestionId !== "number") { | ||
| suggestionId = generateId(); |
There was a problem hiding this comment.
Doesn't this mean that if the user-provided generateId returns a number, we won't call generateId to produce the next id? I'm not sure why just checking for the existence of generateId is insufficient here
There was a problem hiding this comment.
Yeah, this part is a bit ugly. I think I did it this way because I wanted to exhaust the types. The undocumented assumption is that generateId will return string. But! The return type of generateId is string | number because I didn't want to complicate the typing.
It's hard to type "suggestionId is number if generateId is not defined, otherwise string ".
So this is an ugly but valid solution to a typing issue. I didn't spend that much time / brain on it.
In other words: the types of generateId and suggestionId is connected, that expression should always be true if either part of it is true, I've just added an extra guard around that. Yes, the typeof check can be removed if you want.
There was a problem hiding this comment.
Oh... ok. I guess I see. It seems like we should probably enforce that generateId returns a string, at least in the client-facing types, because if someone tries to use, say, a random number, they're probably gonna have a bad time haha. I can do that
d00c99c to
d0e45ec
Compare
3 participants
Hey @smoores-dev! 👋
We're integrating prosemirror-suggest-changes in our application, and we needed to add support for persistent, unique string ids for suggestions so that we can tie additional metadata (e.g. author information, conversation threads) to them.
I'm opening this draft PR as a conversation on whether you'd be open to considering something like this in upstream. The current iteration is a breaking change from your existing schema (it changes the id type from
numbertostring), so we would probably want to have a backwards compatible approach here.Let me know your thoughts -- this current patch works well for us now, but I'm happy to do the additional work to make this mergeable if there's broad interest.