added 'isLocated' flag to operations that receive indexes as arguments.

[amir-arad]

Added the option for the user to supply indexes based on the current state of the string, instead of the original string.

[codecov-io]

Current coverage is 94.73%

Merging #26 into master will increase coverage by +0.29% as of 54742bf

@@            master     #26   diff @@
======================================
  Files            6       6       
  Stmts           18      19     +1
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit             17      18     +1
  Partial          0       0       
  Missed           1       1       

Review entire Coverage Diff as of 54742bf

Powered by Codecov. Updated on successful CI builds.

[amir-arad]

@Rich-Harris what do you think?

[Rich-Harris]

@amir-arad Hi! Thanks for the PR and sorry for the radio silence, been short of time lately.

My initial response is that this isn't the right API – it's a classic example of the Boolean trap. Given that you're essentially trying to do two very different things depending on whether isLocated is true or false, it seems like it would probably be better to create new methods.

I'm curious about the intended use – could you elaborate? We might be able to come up with something better.

[amir-arad]

@Rich-Harris I Would love to hear your feedback and advice!

I'm writing a visitors engine. It's designed function is a fast pre-transpiler that produces intermediate code that will be sent to a transpiler as input. Visitors can delete or insert text at any position and in any order.
I'm using MagicString (specifically the overwrite() method) to sync between source and intermediate texts, and later to translate the transpiler's sourcemap so that the final sourcemap refers directly to the original source code. (I'm planning an optimization PR for MagicString's naive locateOrigin())

A visitor may overwrite a text section while a later visitor may insert text at the beginning or end of that section, using the first or last index of the deleted section as the location of insertion, which causes an error to be thrown by MagiString. This error is part of the tested behaviors and provides helpful feedback, only that in my case I would like to bypass it both for functionality reasons and for performance (so try-catch is not an option).

In such cases I would like to 'snap' the insertion to the edges past the deleted section, so it happens in the nearest valid location. So I have this workaround in place:

execute(actionList){
    actionList.forEach(action => {
        var start = this.locate(action.start);
        var end = this.locate(action.end);
        this.magicString.overwrite(start, end, action.str);
    });
}
private locate(idx) {
    var result = this.magicString.locate(idx);
    if (result === null && idx > 0){
        result = this.magicString.locate(idx-1);
    }
    if (result === null && idx < this.magicString.toString().length){
        result = this.magicString.locate(idx+1);
    }
    if (result === null){
        throw new Error( `Visitor referring an outdated location ${idx}`);
    }
    return this.magicString.locateOrigin(result);
};

As you see, I'm locating each index, correcting it (or throwing an error myself), then locating it's origin so that I can send it to the overrite() method that locates it.
that last call this.magicString.locateOrigin(result) is what I'm trying to avoid.

What do you think?

[Rich-Harris]

Thinking aloud here: it sounds like the need, ultimately, is to be able to update 'patches' after they've been applied. Maybe this would be a more flexible library if it worked that way internally – rather than destructively updating a string internally, and updating the location map with each edit, maybe it non-destructively keeps track of which patches have been applied.

The API could work exactly as it does now, with the same restrictions, except that you could do this sort of thing...

let patch = magicString.findPatch( 42 );
// Patch {
//   start: 40,
//   end: 50,
//   content: 'REPLACEMENT',
//   original: 'abcdefghij'
// }

patch.prepend( 'THIS IS A ' ).append( ' STRING' ).replace( 'REPLACEMENT', 'NEW' );
// Patch {
//   start: 40,
//   end: 50,
//   content: 'THIS IS A NEW STRING',
//   original: 'abcdefghij'
// }

...or it could become more flexible, by losing some of the implementation-mandated restrictions.

It should be more performant as well – no need to adjust every character index that follows an edit (which is only necessary in case you're generating a sourcemap, which isn't always the case).

What do you reckon? Tempted to have a crack at this, it seems like a more sensible design.

[amir-arad]

I was leaning towards writing something in the lines of the patches solution just before I discovered MagicString.
However I'm not sure about the API for traversing the patches. I saw you made PR #30 and I'd love to give it a serious look, I just need to find the time for it.

[Rich-Harris]

@amir-arad yeah, findPatch was more of a straw man than anything. I'm sure we can figure out something better – would welcome any ideas you have. The new patch method takes the same arguments as overwrite, but returns the patch object rather than this, so maybe that's a starting point.

Anyway, good news – I just did some stress testing and as of this PR, making changes is several hundred times faster, and generating sourcemaps (the most time-consuming part) is roughly 25-30x faster – all in all about 35-40x performance improvement. (The test was just replacing every var with differently-sized strings in a 15K LOC file.) It's good to be lazy!

[amir-arad]

👍 for the perf. boost, no surprise there I guess.
I still haven't got to migrating to the new version. as soon as I see that it's working for me, I'll close this PR.
Thanks!