Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

accept strings in Automerge.Text.insertAt #326

Open
josharian opened this issue Mar 10, 2021 · 5 comments · May be fixed by #350
Open

accept strings in Automerge.Text.insertAt #326

josharian opened this issue Mar 10, 2021 · 5 comments · May be fixed by #350
Milestone

Comments

@josharian
Copy link
Contributor

I sometimes have to do bulk inserts into an Automerge.Text, e.g. when importing a new document.

The current API accepts inserts one character at a time: text.insertAt(0, 'h', 'e', 'l', 'l', 'o'). Doing this with bulk inserts I've hit the javascript limit on the number of arguments allowed to a function call (65536).

I'd like it if insertAt also accepted strings and treated them indistinguishably from individual characters: text.insertAt(0, 'hello') (and maybe text.insertAt(0, 'he', 'llo'), not sure).

Or alternatively, a new API that accepted only one single string: text.insertStringAt(0, 'hello').

I'm willing to attempt an implementation if you're open to it.

@ept ept added this to the 1.0 alpha release milestone Apr 12, 2021
@ept
Copy link
Member

ept commented Apr 12, 2021

@josharian Sorry for the delay. I think this is a good idea, and we can make an API change for this in the 1.0 release.

Suggestion: how about we get rid of insertAt, and replace it with insertCharsAt(index, chars) and insertStringAt(index, string)? chars can be an array of characters (not varargs/spread syntax), and string is split into an array of characters using [...string] (see also #334).

The rationale for providing a character-wise insertion method is that if an application wants to use a different way of splitting a string into characters (e.g. by Unicode grapheme cluster rather than by code point), it can do so.

@ept
Copy link
Member

ept commented Apr 12, 2021

If you do a PR for this, please make it relative to the performance branch, so that it goes in the 1.x release series.

@pvh
Copy link
Member

pvh commented Apr 12, 2021

Is there a good reason to make these separate methods rather than doing something pseudo-polymorphic and checking the input for array / string?

@ept
Copy link
Member

ept commented Apr 12, 2021

@pvh Good point, that would be better. So maybe we have a single insertAt method as now, but have it accept an array of characters or a string (which gets split) as second argument?

@lightsprint09 lightsprint09 linked a pull request Apr 30, 2021 that will close this issue
@lightsprint09
Copy link
Member

@pvh @josharian @ept

I implemented a pseudo-polymorphic insertAt in #350.
Please give some feedback and we can iterate on it.

@ept ept modified the milestones: 1.0 alpha release, 1.0 May 5, 2021
geoffreylitt referenced this issue in inkandswitch/peritext Jun 10, 2021
When multiple characters are inserted atomically (eg in a paste),
Prosemirror generates a step with a single insertion string.
But Automerge expects us to give it one character at a time,
so we need to split the character up ourselves.

See for more context:
automerge/automerge#326
echarles pushed a commit to datalayer-externals/automerge-classic-arch that referenced this issue Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants