-
Notifications
You must be signed in to change notification settings - Fork 114
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
Conversation
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 |
Yes, good idea. I've made that change. Sorry for the delay - this one got lost in my inbox |
update
method as safer alternative to overwrite
Can you also update the dts? Thanks |
Also would be great to include a few tests for it. |
update
method as safer alternative to overwrite
update
method as safer alternative to overwrite
@@ -349,6 +349,12 @@ export default class MagicString { | |||
} | |||
|
|||
overwrite(start, end, content, options) { | |||
options = options || {}; | |||
options.overwrite = !options.contentOnly; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, PR welcome
There was a problem hiding this comment.
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
In vitejs/vite#7397 we added
{ contentOnly: true }
to every call tooverwrite
in order to fix vitejs/vite#7365. This could probably be the default. To quote @dummdidumm on that issue: