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

feat: add update method as safer alternative to overwrite #212

Merged
merged 7 commits into from Oct 5, 2022

Conversation

benmccann
Copy link
Contributor

In vitejs/vite#7397 we added { contentOnly: true } to every call to overwrite in order to fix vitejs/vite#7365. This could probably be the default. To quote @dummdidumm on that issue:

The overwrite call removes all prepends/appends that were done on the start/end position if you don't pass in the { contentOnly: true } option as forth argument. I never came across a case where I would want the default behavior, so I'm always setting that option.

@antfu
Copy link
Collaborator

antfu commented Mar 24, 2022

Instead of deprecating it, I feel there is no harm to provide both options for different usage. We could just update the examples to prompt .update more.

@benmccann
Copy link
Contributor Author

Yes, good idea. I've made that change. Sorry for the delay - this one got lost in my inbox

@benmccann benmccann changed the title Add update method and deprecate overwrite Add update method as safer alternative to overwrite Aug 4, 2022
@antfu
Copy link
Collaborator

antfu commented Aug 6, 2022

Can you also update the dts? Thanks

@antfu
Copy link
Collaborator

antfu commented Aug 6, 2022

Also would be great to include a few tests for it.

README.md Outdated Show resolved Hide resolved
@antfu antfu changed the title Add update method as safer alternative to overwrite feat: add update method as safer alternative to overwrite Oct 5, 2022
@antfu antfu merged commit 9a312e3 into Rich-Harris:master Oct 5, 2022
@benmccann benmccann deleted the update branch October 6, 2022 15:49
@@ -349,6 +349,12 @@ export default class MagicString {
}

overwrite(start, end, content, options) {
options = options || {};
options.overwrite = !options.contentOnly;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modifying a user provided options object isn't great DX and unexpected.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, PR welcome

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, you're right. I've sent a PR: #227

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.

SSR transform loses Object.defineProperty in certain cases
3 participants