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

Fix trim bug with intro/outro #144

Merged
merged 1 commit into from
May 31, 2018
Merged

Conversation

guybedford
Copy link
Contributor

This fixes a bug where the intro and outro were being removed on trimmed chunks.

This caused issues where magicSting.appendRight('x').trim() would remove the 'x' part.

@mourner mourner merged commit 5a71147 into Rich-Harris:master May 31, 2018
@lukastaegert
Copy link
Contributor

Well, semantically the original behaviour of magic-string was correct. If you "appendRight" then you tell magic-string that the appended code belongs to the code to the right of the insertion. If there is only whitespace which is removed by "trim" or there is no code at all, then it is correct to remove the appended code.

But then again, I guess this makes things a little easier for rollup. But the new behaviour is inconsistent.

@lukastaegert
Copy link
Contributor

The correct way to add an outro is "appendLeft"

@guybedford
Copy link
Contributor Author

trim() should behave like String.trim() on the output string. So removing content that would otherwise appear in the output seems blatantly wrong.

That said, the concept of using magicString.overwrite(a, b, '') as a Node removal mechanism does have issues in that it won't remove prepended and appended content.

One thing that might be useful here is a magicString.remove(a, b) which would also remove appended and prepended content along with the removal, unlike overwrite.

Alternatively we could look at an magicstring.overwriteOuter(a, b, '') which would overwrite the content of the range, including any appendages of the range.

@lukastaegert
Copy link
Contributor

I think most of our problems with magic-string are related to the fact that magic-string tries to associate appended and prepended code with neighbouring content and remove it together with this code. Because as far as I know this is virtually NEVER what we want because removed nodes are not rendered at all for performance considerations.
Thus I think what we need is a way to insert code in a way so that it is NOT removed together with neighbouring code and this should solve most of our problems.

@aleclarson
Copy link

aleclarson commented Jun 15, 2018

@lukastaegert I agree with your take in the comment above mine. Perhaps expose a chunk(content) function (not a method) that returns a Chunk instance, and extend the existing methods to support chunks in place of content strings.

// Avoid adding the given content to an `intro` or `outro` of some existing chunk.
magicString.appendLeft(index, chunk(content));

Also, the documentation isn't clear enough about the existing behavior. The concepts of intro and outro are completely foreign to users that don't dive into the implementation.

Perhaps all inserted content should get its own Chunk instance by default (though, that's probably less performant and it's a breaking change). That seems like the most predictable behavior.

@guybedford The overwrite method has a contentOnly option, and the docs suggest that overwrite does remove the intro and outro of a chunk by default. Also, it seems that the remove method also removes the intro and outro.

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 this pull request may close these issues.

4 participants