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

[replace] saner default for delimiters #1524

Closed
Hebilicious opened this issue Jun 29, 2023 · 2 comments
Closed

[replace] saner default for delimiters #1524

Hebilicious opened this issue Jun 29, 2023 · 2 comments

Comments

@Hebilicious
Copy link

Hebilicious commented Jun 29, 2023

  • Rollup Plugin Name: replace
  • Rollup Plugin Version: 5.0.2

Expected Behavior / Situation

Delimiters defaults to ['(?<!\\.)\\b', '\\b(?!\\.)']

Actual Behavior / Situation

Delimiters currently defaults to ['\\b', '\\b(?!\\.)']

Modification Proposal

The delimiters defaults are a little too permissive, which means that it's easy to have an error where globalThis.process.env.NODE_ENV gets transformed into globalThis."production" which causes error in dependencies.

See conversation and and example on how it happened with graphql and nitro nuxt/nuxt#21768 (comment)

I believe this fix should prevent other users from having the same issues in the future, and shouldn't break existing behaviours. However I am nowhere near a rollup expert and I could be wrong, so feel free to close this if this will break more things than it will fix.

If you think this is a good idea, I am happy to submit a PR with the implementation.

@shellscape
Copy link
Collaborator

Thanks for the issue. The best route for you would be to make the change and see what tests fail. That would give you (and us) a decent idea of the impact. It'd definitely be considered a breaking change, but that wouldn't stop us from releasing it.

Copy link

stale bot commented Dec 15, 2023

Hey folks. This issue hasn't received any traction for 60 days, so we're going to close this for housekeeping. If this is still an ongoing issue, please do consider contributing a Pull Request to resolve it. Further discussion is always welcome even with the issue closed. If anything actionable is posted in the comments, we'll consider reopening it.

@stale stale bot closed this as completed Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants