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(replace)!: do not escape delimiters #1088

Merged
merged 2 commits into from
Feb 22, 2022

Conversation

guansss
Copy link
Contributor

@guansss guansss commented Jan 9, 2022

Rollup Plugin Name: 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: #904

Description

Fixes #904, now delimiters given by users are no longer escaped, so it's possible to pass ['\\b', '\\b'] to revert to the default behaviour prior to #903.

It's breaking because users may have been relying on current behaviour (bug) to match special characters which are now required to be escaped.

@guansss guansss requested a review from shellscape as a code owner January 9, 2022 18:13
@guansss guansss changed the title Replace delimiters fix(replace)!: do not escape delimiters Jan 9, 2022
@shellscape
Copy link
Collaborator

@guansss apologies for the delay in getting to your PR. it looks like there are a few conflicts to clean up since you submitted it. Please clean those up and we'll give this another look.

@guybedford could you lend your eyeballs to this breaking change?

BREAKING CHANGES: user is now responsible for escaping delimiters
@guansss
Copy link
Contributor Author

guansss commented Feb 22, 2022

Okay, I've rebased it onto master.

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Great work. Looks solid to me. Is the escape function still used elsewhere or can it be removed?

This is a major change so we should bump the semver version.

@guybedford
Copy link
Contributor

Another thing that could be useful is accepting an actual regex for delimiters by value as well eg via if (delimiter instanceof RegEx) kind of thing.

@shellscape shellscape merged commit 5bae547 into rollup:master Feb 22, 2022
@guansss
Copy link
Contributor Author

guansss commented Feb 23, 2022

Thanks @guybedford. Yeah, the escape function is still used on replacement keys.

The delimiters will end up being concatenated into a string to build the regex, so there may be no need to accept a regex.

@guybedford
Copy link
Contributor

@guansss accepting a regex may still solve the original use case of supporting an unescaped input though!

@guansss
Copy link
Contributor Author

guansss commented Feb 24, 2022

@guybedford that makes sense, but IMO it's more like a trick and could be easily done from user-side via something like delimiters: [/\b/.source, /\b/.source].

@guybedford
Copy link
Contributor

@guansss at the end of the day manual regex escaping is work, and something that can be a cause of errors. Making things easier where possible without sacrificing maintainability is the goal in open source!

@guansss
Copy link
Contributor Author

guansss commented Feb 25, 2022

@guybedford agreed, I'm not quite into it though!😄

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.

replace plugin. "delimiters" config is broken by "escape" internal mutation
4 participants