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

feat: allow any JS identifier in define, not ASCII-only #5972

Merged
merged 12 commits into from May 5, 2022

Conversation

jonaskuske
Copy link
Contributor

@jonaskuske jonaskuske commented Dec 6, 2021

Description

The define plugin uses RegExp \b boundaries to make sure that it only replaces standalone identifiers (only _PROD_, not _PROD_USE_DEVTOOLS_). But \b only works with ASCII characters and also not with $, so it doesn't work reliably and worse, there can be false positives leading to broken code:

// { define: { ÜPDATE: '60 * 60 * 24' } } →→ /\bÜPDATE\b/g

const update = ÜPDATE // not replaced, \b boundary is between Ü and P!

const AUTOÜPDATE = update && mode === 'auto' // boundary found between O and Ü!
// replaced, errors:
const AUTO60 * 60 * 24 = update && mode === 'auto'

This PR updates the RegExp to work with any valid JS identifier, so $ and Unicode letters can be included. I also added a couple more tests, both for the newly added $ and Unicode functionality as well as for previously untested functionality:

  • shouldn't substitute replacement within other identifiers (_OTHER_CONST_CONTAINING_PRODUCTION_)
  • shouldn't substitute replacement in property access (someApiResponse._PRODUCTION_)

fix #5956

Additional context

Because the usage of \b for replacements was mentioned in the docs and therefore specced, this technically is a new feature and not a bugfix.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

trailing dots in replacements are not supported in dev,
just prod and only used internally for import.meta.env.
(in dev it's handled by importAnalysisPlugin)
docs/config/index.md Outdated Show resolved Hide resolved
@Shinigami92 Shinigami92 added the p2-nice-to-have Not breaking anything but nice to have (priority) label Dec 7, 2021
Shinigami92
Shinigami92 previously approved these changes Dec 8, 2021
aleclarson
aleclarson previously approved these changes Mar 16, 2022
@jonaskuske jonaskuske dismissed stale reviews from aleclarson and Shinigami92 via 4aecf7d March 27, 2022 13:26
@jonaskuske
Copy link
Contributor Author

Rebased and updated to include the exclusion of (trailing) assignments shipped in 2.8: #5515

@jonaskuske
Copy link
Contributor Author

Greatly simplified the RegExp and fixed the bug where equality operators following an env var would break the replacement. (e.g. process.env.NODE_ENV === 'production')

Tests pass now, and I think this is ready to be merged?

patak-dev
patak-dev previously approved these changes Apr 11, 2022
@patak-dev patak-dev added this to the 3.0 milestone Apr 11, 2022
@patak-dev
Copy link
Member

LGTM, let's wait for other approvals. And IMO we should merge this one as part of the 3.0 beta, just to play safe.

Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
@patak-dev patak-dev merged commit 95eb45b into vitejs:main May 5, 2022
@bluwy bluwy mentioned this pull request Jun 19, 2022
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

define doesn't work with keys starting with $
5 participants