fix(replace)!: issues with nested objects replacements

[o-alexandrov]

Rollup Plugin Name: @rollup/plugin-replace

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

Description

Currently, developers have issues with replacements w/ nested access.
If you attempt to replace typeof window with "object", you would see the following scenarios:

• typeof window will be replaced
• typeof window.document will be replaced, resulting in "object".document (which is an error)
• typeof windowSmth will not be replaced due to a \b boundary

Therefore, it's important to change the default delimiters from:
['\b', '\b']
to:
['\b', '\b(?!\.)']

[shellscape]

Thanks for the PR. Since this does contain a breaking change (any change in default behavior is breaking, even a bug fix), I would encourage you to bring this up as an issue for discussion first. I'm sure this is perceived as a bug, but there's technically nothing incorrect about the current behavior, because the option exists to customize it. Since we already have a delimiters option, it's my position that we need but a documentation improvement, rather than a breaking change which requires the same documentation improvement for users to understand what the new behavior is.

Other maintainers may weigh in on this, but it's my take that we should not merge this change as-is.

[o-alexandrov]

@shellscape I was going to actually report an issue with delimiters also.

• here is an issue with a repro replace plugin. "delimiters" config is broken by "escape" internal mutation #904

[o-alexandrov]

@shellscape as you requested, created an Issue to discuss this breaking fix 😄

• replace plugin. default "delimiters" value is breaking for developers #905

[shellscape]

Ah thanks, I meant more so for the future. Since this is already open and discussion has commenced, the merits of the change can be discussed here.

[o-alexandrov]

apologies for pinging, could you please let me know whether anyone is expected to be looking at this PR and when?

[shellscape]

Please update from upstream/master. We've made some improvements to repo workflows and we'll need them to run against your PR.

[o-alexandrov]

@shellscape rebased and force pushed, please let me know, if I should do anything else to speed up the review

[shellscape]

[jbt]

I just stumbled on this PR after noticing the breaking changes myself in the replace plugin - I'd like to highlight that this change in defaults breaks things where you might want to preserve property / method calls on the thing being replaced.

For me, for example, I have a lot of things that look like:

if (VERSION.startsWith('foo')) {
  // things
}

... where I'm replacing VERSION with a string literal. This change in defaults breaks that behaviour.

I'm aware that all I need to do is change the delimiters I'm using and I'm cool with doing that - but since (as highlighted earlier in this thread) there doesn't seem to have been discussion of the relative merits of different default options, which cases they support and which cases they break - would it be worth opening a discussion to decide the best options here?

If so, I'm happy to create a new issue for discussion. But if not, and if my example is much more edge-case than the typeof window-style replacement, I'm fine to continue manually specifying my own delimiters with the old values

[o-alexandrov]

Personally, I believe it's dangerous to replace VERSION in the case you highlighted by default:

if (VERSION.startsWith('foo')) {
  // things
}

In the case above with the previous default to match VERSION when used with a . or nested access, you would need to know all of your dependencies' code, not to mess up when replacing.