feat(replace): prevent accidental replacement within assignment

[danielroe]

Rollup Plugin Name: @rollup/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

It's possible when replacing process.env values that packages may be writing to these values. This PR adds a negative lookahead test to the generated RegExp to prevent some of these situations.

[shellscape]

Thanks for this PR. Could you go into some more detail as to how this change works and the problem that it solve? Examples would be wonderful. (This will be important information for context for folks that may be looking at this PR via blame)

[danielroe]

@shellscape This PR will prevent replacing strings where they are followed by a single equals sign. For example, where the plugin is called as follows:

replace({
  values: {
    'process.env.DEBUG': 'false'
  }
})

Observe the following code:

// Input
process.env.DEBUG = false
if (process.env.DEBUG == true) {
  //
}
// Before this PR
false = false // this throws an error because false cannot be assigned to
if (false == true) {
  //
}
// After this PR
process.env.DEBUG = false
if (false == true) {
  //
}

[shellscape]

Thanks for the explanation. That's some pretty specific behavior, and not one that I'm sure we want to enable by default. That's one of those "ecosystem gotchas" that I'm sure you're aware of: users rely on a specific behavior and structure code accordingly, only to have that behavior "fixed" in a patch version, causing chaos. I see the value in this change, and it was also raised earlier today in #803. This is a pretty big change that could break some behaviors that folks are relying on, and I wouldn't want to hamstring those folks with no way back, so to speak.

That said, I think it'd be reasonable to make this an option of some sort, defaulting to the original behavior. I would even be willing to output a warning if the option was false or original behavior, indicating that would change in the next major. I wouldn't remove the option in the next major, but just flip the bit.

[danielroe]

@shellscape That seems very reasonable. I've added a preventAssignment option and warning, and happy to rename option or edit warning as you like; just let me know.

[shellscape]

[acmiyaguchi]

What is the process of getting an npm package deployed with new feature changes like this one? This feature is relevant to some issues I'm facing while trying to replace process.browser in certain packages.

[shellscape]

@acmiyaguchi patience. you exercise patience. please refrain from similar replies in the future, it just creates noise.

[bdkjones]

I like this change a lot, but I do have a suggestion:

Because users will now receive a warning if they do not explicitly configure this option, the examples in README should be updated to show explicit configuration of this option in all cases. Someone following the current examples will be surprised at the warning.

[shellscape]

@bdkjones good suggestion. would you be willing to open a PR?

[bdkjones]

@shellscape Sure. I would use true in the examples even though the default is false because I think true is the better value and false is the default simply to avoid breaking changes. Thoughts?