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

fix(reactivity): lose activeEffectScope when effectScope(true) #5575

Merged
merged 3 commits into from Apr 12, 2022

Conversation

a65162
Copy link
Contributor

@a65162 a65162 commented Mar 12, 2022

Vue SFC Playground:

  1. v3.2.30
  2. v3.2.29

v3.2.30 will be lost activeEffectScope and previous version was worked properly.

@netlify
Copy link

netlify bot commented Mar 12, 2022

Deploy Preview for vuejs-coverage ready!

Name Link
🔨 Latest commit 513cc8b
🔍 Latest deploy log https://app.netlify.com/sites/vuejs-coverage/deploys/623af1adf5286f00086718ba
😎 Deploy Preview https://deploy-preview-5575--vuejs-coverage.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Mar 12, 2022

Deploy Preview for vue-next-template-explorer ready!

Name Link
🔨 Latest commit 513cc8b
🔍 Latest deploy log https://app.netlify.com/sites/vue-next-template-explorer/deploys/623af1ad43732c0009729728
😎 Deploy Preview https://deploy-preview-5575--vue-next-template-explorer.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Mar 12, 2022

Deploy Preview for vue-sfc-playground ready!

Name Link
🔨 Latest commit 513cc8b
🔍 Latest deploy log https://app.netlify.com/sites/vue-sfc-playground/deploys/623af1addb2cd900086d7d96
😎 Deploy Preview https://deploy-preview-5575--vue-sfc-playground.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@a65162 a65162 changed the title fix(reactivity): lose activeEffectScope when new effectScope(true) fix(reactivity): lose activeEffectScope when effectScope(true) Mar 12, 2022
@a65162 a65162 force-pushed the fix/effectScope-parent-is-undefined branch 2 times, most recently from ab934c0 to c7bb868 Compare Mar 12, 2022
@a65162
Copy link
Contributor Author

a65162 commented Mar 18, 2022

Does any maintainer want to review this PR?

@LinusBorg
Copy link
Member

as you may have noticed, we have quite a backlog of PRs, so we are thankful for your patience.

@LinusBorg LinusBorg added 🔍 review needed This PR requires a review by a member feat: reactivity labels Mar 18, 2022
@posva posva added the p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. label Mar 18, 2022
Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

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

Can you add a regression test for this?

packages/reactivity/src/effectScope.ts Outdated Show resolved Hide resolved
@a65162 a65162 force-pushed the fix/effectScope-parent-is-undefined branch from c7bb868 to f3314fe Compare Mar 19, 2022
@a65162
Copy link
Contributor Author

a65162 commented Mar 19, 2022

Can you add a regression test for this?

Sure.

@a65162 a65162 requested a review from posva Mar 19, 2022
posva
posva approved these changes Mar 23, 2022
@LinusBorg LinusBorg added this to Open for Review in Next Patch Mar 23, 2022
@qq15725
Copy link

qq15725 commented Apr 4, 2022

off() is the same problem.

@qq15725
Copy link

qq15725 commented Apr 4, 2022

Maybe it would be better to just change these two positions.

  constructor(detached = false) {
    // 👇🏻 this line
    this.parent = activeEffectScope
    if (!detached && activeEffectScope) {
      this.index =
        (activeEffectScope.scopes || (activeEffectScope.scopes = [])).push(
          this
        ) - 1
    }
  }
      // nested scope, dereference from parent to avoid memory leaks
      // 👇🏻 this line
      if (this.index !== undefined && this.parent && !fromParent) {
        // optimized O(1) removal
        const last = this.parent.scopes!.pop()
        if (last && last !== this) {
          this.parent.scopes![this.index] = last
          last.index = this.index
        }
      }

@posva
Copy link
Member

posva commented Apr 5, 2022

@qq15725 do you have a failing test case so we can add it?

@qq15725
Copy link

qq15725 commented Apr 5, 2022

@posva

Vue SFC Playground: 3.2.31

<script setup>
  import { effectScope, onScopeDispose } from 'vue'
  effectScope(true).off()
  onScopeDispose(() => undefined)
</script>

Vue warn

[Vue warn] onScopeDispose() is called when there is no active effect scope to be associated with.

@qq15725
Copy link

qq15725 commented Apr 5, 2022

test case:

it('getCurrentScope() stays valid when offed a detached nested EffectScope', () => {
  const parentScope = new EffectScope()

  parentScope.run(() => {
    const currentScope = getCurrentScope()
    expect(currentScope).toBeDefined()
    const detachedScope = new EffectScope(true)
    detachedScope.off()

    expect(getCurrentScope()).toBe(currentScope)
  })
})

@posva
Copy link
Member

posva commented Apr 5, 2022

Thanks. You don't call off on an effectScope, you call stop() (https://vuejs.org/api/reactivity-advanced.html#effectscope) so that will never happen in a real scenario.

@LinusBorg LinusBorg moved this from Open for Review to High Priority in Next Patch Apr 10, 2022
@yyx990803 yyx990803 merged commit 0a301d4 into vuejs:main Apr 12, 2022
8 checks passed
Next Patch automation moved this from High Priority to Done Apr 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. feat: reactivity 🔍 review needed This PR requires a review by a member
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants