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(define): prevent assignment #5515

Merged
merged 1 commit into from
Dec 28, 2021

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Nov 3, 2021

Description

Fixes #5269
Fixes #4271
Fixes #5278
Fixes #4887

Prevent define replacement if has trailing assignments

Additional context

Should this be a new config option? It could be breaking, but in rare edge cases.


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.

@Shinigami92 Shinigami92 added the p3-minor-bug An edge case that only affects very specific usage (priority) label Nov 3, 2021
@patak-dev
Copy link
Member

Im not sure we should do this. The users in the linked issues should use a constant name tgat doesnt conflict with their code. With this change, the local variable isnt replaced but their use later in the scope is replaced, so potentially there could be inconsistencies because of this. IMO if wexare not going to do proper AST replacement, we should keep define simple and continue to ask users to choose non-conflicting names

@bluwy
Copy link
Member Author

bluwy commented Nov 3, 2021

Wouldn't the inconsistency be intentional? I believe this is how @rollup/plugin-replace works too according to its docs example, and it's recommended true. FWIW the regex used is the same as well. I think it's still a fair change to guard users from the odd error message, and in most case the user would know the risk of using define and its inconsistency.

@patak-dev
Copy link
Member

Thanks for the pointers @bluwy, looks like Rollup didn't change it right away to prevent a breaking change then. I would vote to add this without an option as you did in this PR to align with future versions of the official rollup define plugin (I don't think we should have the complexity of options on our side). I'll bring this PR to the next team meeting so we can discuss it there

@patak-dev patak-dev added this to the 2.8 milestone Dec 3, 2021
@haoqunjiang haoqunjiang mentioned this pull request Dec 4, 2021
7 tasks
@patak-dev
Copy link
Member

We talked in the last team meeting about this, let's merge it in the 2.8 beta so the ecosystem has a bit of time to test it

@patak-dev patak-dev merged commit 6d4ee18 into vitejs:main Dec 28, 2021
@btea
Copy link
Collaborator

btea commented Dec 29, 2021

Perhaps the import/export variables and file names should also be ignored?

@bluwy bluwy deleted the feat-define-prevent-assignment branch December 29, 2021 07:43
@patak-dev
Copy link
Member

@btea would you create a new feature request or PR to discuss further changes?

@btea
Copy link
Collaborator

btea commented Dec 29, 2021

There is one more question, I don’t know if my understanding is correct.

According to the current modification, the variable declaration will be prevented from being replaced, but the use of the variable does not prevent the replacement, which will cause the packaged code to get the wrong value of the variable.

@btea
Copy link
Collaborator

btea commented Dec 29, 2021

@btea would you create a new feature request or PR to discuss further changes?

Maybe I should create a new feature to discuss it?

@bluwy
Copy link
Member Author

bluwy commented Dec 29, 2021

There is one more question, I don’t know if my understanding is correct.

According to the current modification, the variable declaration will be prevented from being replaced, but the use of the variable does not prevent the replacement, which will cause the packaged code to get the wrong value of the variable.

If you mean for code like this:

const __PROD__ = false

console.log(__PROD__) // if we replace __PROD__ as true, this will log true, instead of the from the variable -- false

I think this behaviour would be intentional from the user point of view, as from the linked issues.

Perhaps the import/export variables and file names should also be ignored?

This sounds like a fair thing to support too, I think we should create a new feature issue for it.

@btea
Copy link
Collaborator

btea commented Dec 29, 2021

I think this behaviour would be intentional from the user point of view, as from the linked issues.

Yeah, you are right.

This sounds like a fair thing to support too, I think we should create a new feature issue for it.

ok.

poyoho added a commit to poyoho/vite that referenced this pull request Dec 30, 2021
commit d856c4b
Author: Anthony Fu <[email protected]>
Date:   Thu Dec 30 00:25:59 2021 +0800

    fix(ssr): move `vite:ssr-require-hook` after user plugins (vitejs#6306)

commit b45f4ad
Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Date:   Wed Dec 29 14:49:15 2021 +0100

    chore(deps): update all non-major dependencies (vitejs#6185)

commit 4d75b2e
Author: Niputi <[email protected]>
Date:   Wed Dec 29 14:48:13 2021 +0100

    feat: catch postcss error messages (vitejs#6293)

commit f68ed8b
Author: Bogdan Chadkin <[email protected]>
Date:   Wed Dec 29 16:40:13 2021 +0300

    fix: replace execa with cross-spawn (vitejs#6299)

commit 44bb4da
Author: patak <[email protected]>
Date:   Wed Dec 29 12:51:46 2021 +0100

    chore(deps): update to esbuild fixed at 0.14.3 (vitejs#5861)

commit 9ad7c55
Author: patak <[email protected]>
Date:   Wed Dec 29 11:32:49 2021 +0100

    deps: update to typescript 4.5.4 (vitejs#6297)

commit 1da104e
Author: Aron Griffis <[email protected]>
Date:   Wed Dec 29 02:50:19 2021 -0500

    fix: don't force terser on non-legacy (fix vitejs#6266) (vitejs#6272)

commit 5279de6
Author: ygj6 <[email protected]>
Date:   Wed Dec 29 05:30:47 2021 +0800

    feat: import.meta.glob support ?raw (vitejs#5545)

commit 6d4ee18
Author: Bjorn Lu <[email protected]>
Date:   Wed Dec 29 04:27:49 2021 +0800

    feat(define): prevent assignment (vitejs#5515)

commit ac3f434
Author: Bogdan Chadkin <[email protected]>
Date:   Tue Dec 28 23:26:46 2021 +0300

    fix: upgrade postcss-modules (vitejs#6248)

commit 5a111ce
Author: Bogdan Chadkin <[email protected]>
Date:   Tue Dec 28 23:23:42 2021 +0300

    fix: replace chalk with picocolors (vitejs#6277)

commit 7e3e84e
Author: patak-dev <[email protected]>
Date:   Tue Dec 28 15:07:10 2021 +0100

    release: v2.7.9

commit 83ad7bf
Author: Anthony Fu <[email protected]>
Date:   Tue Dec 28 21:52:41 2021 +0800

     fix: revert vitejs#6251 (vitejs#6290)

    This reverts commit 49da986.

commit 1cbf0e1
Author: Cristian Pallarés <[email protected]>
Date:   Tue Dec 28 11:30:42 2021 +0100

    test: fix test typo (vitejs#6285)

commit d13ced5
Author: patak-dev <[email protected]>
Date:   Tue Dec 28 09:40:48 2021 +0100

    release: v2.7.8

commit dcb1df4
Author: itibbers <[email protected]>
Date:   Tue Dec 28 16:30:32 2021 +0800

    docs: add frontmatters to fix __VP_STATIC_START__ (vitejs#6283)

commit 60ce7f9
Author: Anthony Fu <[email protected]>
Date:   Tue Dec 28 16:20:12 2021 +0800

    fix(ssr): capture scope declaration correctly (vitejs#6281)

commit eb08ec5
Author: Niputi <[email protected]>
Date:   Tue Dec 28 06:10:37 2021 +0100

    chore: remove acorn plugins (vitejs#6275)

commit 64b1595
Author: zhangenming <[email protected]>
Date:   Tue Dec 28 11:52:51 2021 +0800

    chore(create-vite): add more gitignore (vitejs#6247)

commit 49da986
Author: sanyuan <[email protected]>
Date:   Tue Dec 28 05:30:54 2021 +0800

    fix: seperate source and dep for dymamic import after build (vitejs#6251)

commit 394539c
Author: Bogdan Chadkin <[email protected]>
Date:   Tue Dec 28 00:29:23 2021 +0300

    fix: upgrade to launch-editor with picocolors (vitejs#6209)

commit 40e3f73
Author: Shinigami <[email protected]>
Date:   Mon Dec 27 11:42:24 2021 +0100

    chore: fix link (vitejs#6269)

commit e7306b5
Author: Shinigami <[email protected]>
Date:   Mon Dec 27 11:22:44 2021 +0100

    chore: update bug report issue template (vitejs#6263)

commit 1f945f6
Author: Aaron Bassett <[email protected]>
Date:   Sun Dec 26 15:28:47 2021 -0500

    fix(html): show error overlay when parsing invalid file (vitejs#6184)

commit 1d722c5
Author: patak-dev <[email protected]>
Date:   Sun Dec 26 06:35:04 2021 +0100

    release: v2.7.7

commit 2e3fe59
Author: Anthony Fu <[email protected]>
Date:   Sun Dec 26 13:13:24 2021 +0800

    fix(ssr): transform class props (vitejs#6261)

commit 1a6e2da
Author: ygj6 <[email protected]>
Date:   Sat Dec 25 18:24:44 2021 +0800

    docs: typescript tips for using Type-Only Imports and Export (vitejs#6260)

commit 6a47083
Author: Haoqun Jiang <[email protected]>
Date:   Fri Dec 24 14:02:43 2021 +0800

    fix: update the vue version in the error message (vitejs#6252)

commit 485e298
Author: Anthony Fu <[email protected]>
Date:   Thu Dec 23 21:45:13 2021 +0800

    fix(ssr): nested destucture (vitejs#6249)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
4 participants