fix: 🐛 define plugin not ignore file names by Dunqing · Pull Request #6340 · vitejs/vite

Dunqing · 2021-12-31T06:36:48Z

Description

When we use import * as from "CONSTANTS" or export * from "xxxx/CONSTANTS" statements, define plugin will replace CONSTANTS, This behavior is incorrect.

I add some boundary processing to solve this problem, it is not allowed to replace variables when there are "double quotes, single quotes, back quotes, slashes" in front of the CONSTANTS.

Additional context

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.

bluwy

This seems to partially fix #6340. Another case mentioned was with:

import __SOME_VAR__ from "./something"

and

export { __SOME_VAR__ }

These are rare cases though which can be handled in other PRs. Side note: I'm hoping we're not re-inventing @rollup/plugin-replace here 😅

bluwy · 2021-12-31T16:05:42Z

packages/vite/src/node/plugins/define.ts

      // Do not allow preceding '.', but do allow preceding '...' for spread operations
-      '(?<!(?<!\\.\\.)\\.)\\b(' +
-        Object.keys(replacements)
-          .map((str) => {
-            return str.replace(/[-[\]/{}()*+?.\\^$|]/g, '\\$&')
-          })
-          .join('|') +
-        // prevent trailing assignments
-        ')\\b(?!\\s*?=[^=])',
+      `(?<!${characters})(?<!(?<!\\.\\.)\\.)\\b(${replacementsStr})\\b(?!${characters})(?!\\s*?=[^=])`,
+      // prevent trailing assignments


I think we should try to split this regex to multiple lines so the comments match, and would be more readable too.

btea · 2022-01-01T03:35:44Z

packages/vite/src/node/plugins/define.ts

      ...processEnv
    }

+    const characters = '[\'"`\\/]'


Suggested change

const characters = '[\'"`\\/]'

const characters = '[\'"`\\/_-]'

For example, the following code

import * from "xxxx/xx(_|-)PATH

Dunqing · 2022-01-01T04:05:27Z

This seems to partially fix #6340. Another case mentioned was with:
import __SOME_VAR__ from "./something"
and
export { __SOME_VAR__ }
These are rare cases though which can be handled in other PRs. Side note: I'm hoping we're not re-inventing @rollup/plugin-replace here 😅

In my opinion, rare cases can only be solved by ast, but the performance will be reduced, so we can only consider most cases😆

bluwy · 2022-01-01T14:14:43Z

In my opinion, rare cases can only be solved by ast, but the performance will be reduced, so we can only consider most cases

You're right. I'm trying to think of ways to solve those cases, and there would always be edge cases around it unless we have the AST. I'd agree that we should keep it simple then and don't handle those.

To be fair, also handling define for filenames feels like an edge case too that can (slightly) hurt performance. Chances of this happening sounds really small, as file names are usually ./foo/bar and not __FOO_BAR__. Don't take this in a bad way though 😅 But I'm a bit skeptical if we do need this.

Dunqing · 2022-01-01T15:05:18Z

You're right. I'm trying to think of ways to solve those cases, and there would always be edge cases around it unless we have the AST. I'd agree that we should keep it simple then and don't handle those.

To be fair, also handling define for filenames feels like an edge case too that can (slightly) hurt performance. Chances of this happening sounds really small, as file names are usually ./foo/bar and not __FOO_BAR__. Don't take this in a bad way though 😅 But I'm a bit skeptical if we do need this.

You're right, too.
We might need a delimiters option like @rollup/replace-plugin, but that might increase the cost of using it.

bluwy

Putting my approval as the code looks good to me. But my question still stands that whether we want to cover this extra edge case.

gluck · 2022-01-03T16:05:23Z

This PR ssems to fix some weird behavior I highlighted in this discussion: #5725
E.g. no longer replacing values in string (well at start/end of string at least, but I agree it's difficult to do better without AST)

Maybe update the define docs (docs/config.index.md) to reflect the change ?

patak-cat · 2022-01-03T16:16:50Z

We can discuss this one in the next team meeting, but as you are already discussing, we are going to end up with a never-ending game of updating the regex and fixing "bugs". The docs state that you should use unique values like __MY_VERY_UNIQUE_CONSTANT__ as define is a simple replace operation so we need to place the limit somewhere and stick with it.

patak-cat · 2022-02-25T16:54:13Z

@1247748612 we talked about this PR today and we decided that we should merge this fix. Thanks for looking into the issue. We should wait until the 2.9 beta window (probably starting next week) so we get some extra testing. Would you check the conflicts in the PR?

Dunqing · 2022-02-26T03:26:38Z

@1247748612 we talked about this PR today and we decided that we should merge this fix. Thanks for looking into the issue. We should wait until the 2.9 beta window (probably starting next week) so we get some extra testing. Would you check the conflicts in the PR?

Thanks to the team for the discussion on this pr, I have resolved the conflict. I may also need to add some test cases for this pr.

patak-cat · 2022-02-26T03:40:51Z

Thanks again. I think the added test cases are enough to merge it but welcome to add more cases and improve the suite

Co-authored-by: patak-dev <matias.capeletto@gmail.com>

patak-cat · 2022-03-04T20:18:54Z

@1247748612 we are reverting this PR as it causing regressions in Svelte and Vitest. We could work in another PR to implement this feature, but we should see how to simplify the regex and add more tests before moving forward. Thanks again for your work exploring this solution.

Dunqing · 2022-03-08T06:12:49Z

@1247748612 we are reverting this PR as it causing regressions in Svelte and Vitest. We could work in another PR to implement this feature, but we should see how to simplify the regex and add more tests before moving forward. Thanks again for your work exploring this solution.

I don't know why this is causing this, I'll try to fix it and submit the pr again after simplifying the regex. Thanks you again.

dengqing added 4 commits December 31, 2021 14:33

fix: 🐛 define plugin not ignore file names

f01642f

fix: 🐛 test error

a3566f9

fix: 🐛 use template literals

2d1e700

fix: 🐛 errors caused by spaces

df12ddb

bluwy reviewed Dec 31, 2021

View reviewed changes

btea reviewed Jan 1, 2022

View reviewed changes

chore: 🤖 split regex to multiple line and add more boundaries

87800d6

bluwy previously approved these changes Jan 1, 2022

View reviewed changes

patak-cat added the p3-minor-bug An edge case that only affects very specific usage (priority) label Feb 13, 2022

patak-cat added the inconsistency Inconsistency between dev & build label Feb 25, 2022

patak-cat added this to the 2.9 milestone Feb 25, 2022

merge main

4fc4a9d

Dunqing dismissed bluwy’s stale review via 4fc4a9d February 26, 2022 03:15

patak-cat approved these changes Mar 3, 2022

View reviewed changes

patak-cat merged commit 7215a03 into vitejs:main Mar 3, 2022

Dunqing deleted the fix/define-plugin-ignore branch March 3, 2022 09:20

dominikg mentioned this pull request Mar 4, 2022

fix: revert #6340, definePlugin tests, warning box #7174

Merged

12 tasks

patak-cat added a commit to dominikg/vite that referenced this pull request Mar 4, 2022

fix: revert vitejs#6340

042f79e

patak-cat added a commit that referenced this pull request Mar 4, 2022

fix: revert #6340, definePlugin tests, warning box (#7174)

6cb0647

Co-authored-by: patak-dev <matias.capeletto@gmail.com>

	const characters = '[\'"`\\/]'
	const characters = '[\'"`\\/_-]'

Uh oh!

Conversation

Dunqing commented Dec 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional context

What is the purpose of this pull request?

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

Uh oh!

bluwy left a comment

Choose a reason for hiding this comment

Uh oh!

bluwy Dec 31, 2021

Choose a reason for hiding this comment

Uh oh!

btea Jan 1, 2022

Choose a reason for hiding this comment

Uh oh!

Dunqing commented Jan 1, 2022

Uh oh!

bluwy commented Jan 1, 2022

Uh oh!

Dunqing commented Jan 1, 2022

Uh oh!

bluwy left a comment

Choose a reason for hiding this comment

Uh oh!

gluck commented Jan 3, 2022

Uh oh!

patak-cat commented Jan 3, 2022

Uh oh!

patak-cat commented Feb 25, 2022

Uh oh!

Dunqing commented Feb 26, 2022

Uh oh!

patak-cat commented Feb 26, 2022

Uh oh!

patak-cat commented Mar 4, 2022

Uh oh!

Dunqing commented Mar 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Dunqing commented Dec 31, 2021 •

edited

Loading