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: adding missing reactivity api from vue-next #311

Merged
merged 35 commits into from Jun 2, 2020

Conversation

pikax
Copy link
Member

@pikax pikax commented Apr 19, 2020

This plugin is falling behind on the reactivity API

This PR is aimed to make the reactivity API close as possible to vue-next.

Current problems

  • effect not ported
  • readonly not ported
  • makeRaw this is quite cheat, since the original object gets change, having this doesn't do much
  • reactive([]) - because of the v2 reactivity system arrays cannot be reactive, should be warn?

Breaking

  • Watch effect now follows the same behaviour as v3

Added

  • shallowReactive
  • shallowRef

Changes

  • reactive marks all possible objects as reactive to conform to vue 3
  • set marks reactive

@pikax pikax changed the title feat: bring API to vue-3 compatible [WIP] feat: bring API to vue-3 compatible Apr 19, 2020
implementation is wrong, but it just to have something kinda working, suggestions and help is welcome :)
# Conflicts:
#	src/apis/state.ts
#	src/reactivity/index.ts
#	src/reactivity/ref.ts
@pikax pikax changed the title [WIP] feat: bring API to vue-3 compatible [WIP] feat: adding missing reactivity api from vue-next Apr 19, 2020
This was referenced Apr 24, 2020
@LinusBorg LinusBorg self-requested a review May 9, 2020 13:18
@LinusBorg
Copy link
Member

This looks like great work!

I'll need a bit more time to properly review this. I'm on it tough!

@LinusBorg LinusBorg self-assigned this May 11, 2020
src/apis/watch.ts Outdated Show resolved Hide resolved
@LinusBorg
Copy link
Member

LinusBorg commented May 16, 2020

So I have done a review.

  • changes to reactivity look fine
  • tests: We now have a lot of tests where we switched to { immediate: true } (since that's how watch behaves now) and seemingly very little are left testing the now-default {immediate: true } behaviour. But we still have everything covered, I think. just feels weird to have a majority of the tests use the non-default.
  • Types: It's awesome that you took the extra steps and renamed everything to be in line with Vue 3, that looks like it was a lot of work and is highly apprechiated. For the really complex types that you ported from the Vue 3 codebase I can say little as they are a bit over my head for the most part 😅

So from my perspective this looks fine!

@pikax
Copy link
Member Author

pikax commented May 16, 2020

I tried not to make too many changes to the current tests, focused more on porting the tests from vue-next.

Hopefully this types will improve, and they were basically copy&paste from vue-next so updating them in the future should be straight forward, I'm also thinking in bringing the typescript type tests over, but waiting for the typescript 3.9 PR to be merged and the mixins

Copy link
Member

@antfu antfu left a comment

Choose a reason for hiding this comment

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

LGTM. Let me know when you feel it's ready, I think I am able to help with merging.

I also notice this already covered #300. I will close it in favor of this.

Copy link
Member

@antfu antfu left a comment

Choose a reason for hiding this comment

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

There seems to have some uncaptured warnings for npm test which I think should make the tests failed but it didn't. https://github.com/vuejs/composition-api/pull/311/checks?check_run_id=723085160 I will try to investigate a bit.

EDIT: made a PR pikax#2

@pikax
Copy link
Member Author

pikax commented Jun 2, 2020

@antfu sorry missed your comment, didn't noticed the errors! Thank you

expect(fn).toHaveBeenCalledTimes(1);
});

// it('onBeforeUnmount', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Comment this test because couldn't make this work, this was brought from vue-next not sure if we should fix this

// expect(fn).toHaveBeenCalledTimes(1);
// });

// it('onUnmounted', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

ditto

Copy link
Member

@antfu antfu left a comment

Choose a reason for hiding this comment

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

LGTM and let's get it merged! Thanks for the great work!

@antfu antfu merged commit 7701ff8 into vuejs:master Jun 2, 2020
@pikax pikax deleted the feat/vue3_API branch June 2, 2020 20:29
This was referenced Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants