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.

Sign up for GitHub

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

Jump to bottom

fix(reactivity-transform): ensure that macros comply with the original semantics (#6312) #6944

Closed
wants to merge 15 commits into from
Closed

fix(reactivity-transform): ensure that macros comply with the original semantics (#6312) #6944

wants to merge 15 commits into from

Conversation

tuchg
Copy link

@tuchg tuchg commented Oct 25, 2022 •
edited
Loading

unwrap the code form the macro($()、$$())

code:

const r = $ref(1)
$$(r)

now:

const r = _ref(1)
(r)

// ⬇️ formatted to
const r = _ref(1)(r);
// ❌ cannot access 'r' before initialization

RFC & after fix:

const r = _ref(1)
r

// ⬇️ formatted to
const r = _ref(1);
r;

@netlify
Copy link

netlify bot commented Oct 25, 2022 •
edited
Loading

❌ Deploy Preview for vuejs-coverage failed.

Name Link
🔨 Latest commit 309a08b
🔍 Latest deploy log https://app.netlify.com/sites/vuejs-coverage/deploys/636afb2eff24ed0008da10d0

@baiwusanyu-c
Copy link
Member

nice！

@tuchg
Copy link
Author

tuchg commented Oct 27, 2022

@sxzz hi guys, do u have a minute to review this pr?

sxzz
sxzz reviewed Oct 27, 2022
View reviewed changes
Copy link
Member

@sxzz sxzz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It will break existing code. We should keep expressions in backets.

Before

After

@tuchg
Copy link
Author

tuchg commented Oct 27, 2022 •
edited
Loading

It will break existing code. We should keep expressions in backets.

Before

After

wow😮..... thx, i can to retain the expressions in brackets. but, this sence really exists and make sense in unref?

i try to $ref

@tuchg tuchg requested review from sxzz and removed request for sxzz October 27, 2022 13:21
@tuchg tuchg requested a review from sxzz October 27, 2022 13:25
sxzz
sxzz reviewed Oct 27, 2022
View reviewed changes
Copy link
Member

@sxzz sxzz left a comment

Choose a reason for hiding this comment

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

Still error... It's a pretty troubling example.
https://deploy-preview-6944--vue-sfc-playground.netlify.app/#eNqlUMtOwzAQ/BXL6gHU2OFQIREKEv/hS0i2JciP1XqTKory76wbhBAce/M8PDPaRb8h2mkE3ehj7mhAVhl4xFcXPbDq0hhZvaiHZxe7FHPyYH063135exd3u+2531ebV7hjvQVJhACGgL5lEKTUsiiCPHpW61qMP6Ku9BAwEZvQov3MKcqgpXxx30J2ulFXpnCyuGCnP5gxN3XdA/o0GySYBriYx6fDwRhxmXzqjHTMZ5J5vY3AfjjNtkWsRbYkm4cAFnIw75QuGUjqna5+NdVCTkCGIPZAQLc3/wn8117K5UCrXr8ABJKTFw==

@tuchg tuchg changed the title fix(reactivity-transform): ensure macro  comply with the original semantic (#6312) fix(reactivity-transform): ensure that macros comply with the original semantics (#6312) Oct 27, 2022
@tuchg tuchg requested a review from sxzz October 27, 2022 14:27
sxzz
sxzz reviewed Oct 27, 2022
View reviewed changes
Copy link
Member

@sxzz sxzz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

😖Another case

packages/reactivity-transform/src/reactivityTransform.ts Outdated Show resolved Hide resolved
sxzz
sxzz reviewed Oct 28, 2022
View reviewed changes
Copy link
Member

@sxzz sxzz left a comment

Choose a reason for hiding this comment

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

Another case

sxzz
sxzz reviewed Oct 28, 2022
View reviewed changes
Copy link
Member

@sxzz sxzz left a comment

Choose a reason for hiding this comment

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

😅 Such a huge number of edge cases. We should also handle spaces.

Another case

packages/reactivity-transform/src/reactivityTransform.ts Outdated Show resolved Hide resolved
sxzz
sxzz reviewed Oct 28, 2022
View reviewed changes
Copy link
Member

@sxzz sxzz left a comment

Choose a reason for hiding this comment

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

🙇🏻 Sorry, but...
Another case

sxzz
sxzz requested changes Oct 28, 2022
View reviewed changes
Copy link
Member

@sxzz sxzz left a comment

Choose a reason for hiding this comment

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

Case 2

@tuchg tuchg requested a review from sxzz October 29, 2022 02:20
sxzz
sxzz reviewed Oct 29, 2022
View reviewed changes
Copy link
Member

@sxzz sxzz left a comment

Choose a reason for hiding this comment

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

❤️ Great! Thanks for your contribution!

Could you please add more tests about the examples I provided?

packages/reactivity-transform/src/reactivityTransform.ts Outdated Show resolved Hide resolved
@tuchg
Copy link
Author

tuchg commented Oct 29, 2022 •
edited
Loading

❤️ Great! Thanks for your contribution!

Could you please add more tests about the examples I provided?

i think the current tests should already cover all for you provided ?

@tuchg tuchg requested a review from sxzz October 29, 2022 04:20
sxzz
sxzz approved these changes Oct 29, 2022
View reviewed changes
Copy link
Member

@sxzz sxzz left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@tuchg
Copy link
Author

tuchg commented Nov 6, 2022

LGTM, thanks!

hi, it seems to be forgotten 😭 , could you request a maintainer to approve or review it? (first-time contributor 👀

@sxzz
Copy link
Member

sxzz commented Nov 7, 2022

Please be patient, just wait.

tuchg added 4 commits November 9, 2022 08:56
@tuchg
Merge branch 'main' into fix/rt-macro
309a08b 
# Conflicts:
#	packages/reactivity-transform/__tests__/__snapshots__/reactivityTransform.spec.ts.snap
@tuchg
fix: update snap
1b86226
@tuchg
fix: code style
b664c09
@tuchg
Merge branch 'main' into fix/rt-macro
fef27ca 
# Conflicts:
#	packages/reactivity-transform/src/reactivityTransform.ts
@tuchg
Copy link
Author

tuchg commented Nov 9, 2022

@yyx990803 hi, the code-style issues and conflict has been solved

sxzz
sxzz reviewed Nov 9, 2022
View reviewed changes
Copy link
Member

@sxzz sxzz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Could you please revert the format commit? (Just run pnpm run format to fix that)
It causes lots of unrelated changes.

@tuchg tuchg requested a review from sxzz November 9, 2022 03:24
sxzz
sxzz approved these changes Nov 9, 2022
View reviewed changes
Copy link
Member

@sxzz sxzz left a comment

Choose a reason for hiding this comment

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

LGTM

@sxzz sxzz added ready to merge The PR is ready to be merged. 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. labels Nov 12, 2022
@yyx990803 yyx990803 closed this in e06d3b6 Nov 14, 2022
@yyx990803
Copy link
Member

yyx990803 commented Nov 14, 2022 •
edited
Loading

Thanks a lot for the PR and the discovery of the edge cases. However the implementation here is missing the root cause of the problem, leading it to be unnecessarily complicated.

The missing semicolon issue only arises when the CallExpression is the first thing on a newline. This means

  1. The case does not exist for $ because it is only allowed in variable declarations.
  2. We can detect it for $$ by checking whether the CallExpression is the child of an ExpressionStatement.

With that we already rule out many of the edge cases this PR is attempting to account for. The only remaining case is avoiding the semi insertion when there are non-whitespace between CallExpression and the previous newline.

I reused the test cases from this PR to ensure all cases found here are covered.

yyx990803
yyx990803 reviewed Nov 14, 2022
View reviewed changes
packages/reactivity-transform/src/reactivityTransform.ts
s.remove(node.callee.start! + offset, bracketStart)

if (node.arguments.length === 1) {
const bracketEnd = node.arguments.at(-1)!.end! + offset
Copy link
Member

Choose a reason for hiding this comment

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

A side note: Array.prototype.at is only available in Node 16.6+.

Even in Node-only code branches we need to be conservative about using new language features since this can implicitly break user builds running on Node versions below 16.6.

@sxzz sxzz mentioned this pull request Jan 4, 2023
Closed
chrislone pushed a commit to chrislone/core that referenced this pull request Feb 4, 2023
@yyx990803
fix(reactivity-transform): fix $$ escape edge cases
24ecdbd 
fix vuejs#6312
close vuejs#6944
zhangzhonghe pushed a commit to zhangzhonghe/core that referenced this pull request Apr 12, 2023
@yyx990803 @zhangzhonghe
fix(reactivity-transform): fix $$ escape edge cases
21ec81e 
fix vuejs#6312
close vuejs#6944
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yyx990803 yyx990803 yyx990803 left review comments

@sxzz sxzz sxzz approved these changes

Assignees
No one assigned
Labels
🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. ready to merge The PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Reactivity Transform] $$ breaks the original semantics
4 participants
@tuchg @baiwusanyu-c @sxzz @yyx990803