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

chore(dep/vue) bump peerDep composition-api to 0.6 #1253

Merged

Conversation

darrenjennings
Copy link
Contributor

  • breaking changes in composition-api 0.6.x, so I've adapted the code to conform. This is not backwards compat with previous version of @xstate/vue but peerDependencies have been updated. Luckily the changes are more in-line with Vue 3, so this will make it easier to upgrade to Vue 3 when needed.
  • Uses new watcher option since it was changed to be lazy by default
  • now using shallow ref since xstate objects should not be refs deeply

- breaking changes in 0.6.x, adapated code. Will make it easier for
this library to also be updated to Vue 3.
- Uses new watcher option since it was changed to be lazy by default
- now using shallow ref since xstate objects should not be refs deeply
@changeset-bot
Copy link

changeset-bot bot commented Jun 20, 2020

🦋 Changeset is good to go

Latest commit: 7560f68

We got this.

This PR includes changesets to release 1 package
Name Type
@xstate/vue Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 20, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 7560f68:

Sandbox Source
sweet-platform-4u1v6 Configuration
kind-agnesi-8ynhy Configuration

@@ -57,7 +57,7 @@
"devDependencies": {
"@testing-library/jest-dom": "^4.2.4",
"@testing-library/vue": "^4.1.0",
"@vue/composition-api": "^0.3.4",
"@vue/composition-api": "~0.6.5",
Copy link
Member

Choose a reason for hiding this comment

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

any reason why this is using ~ over ^?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, changing back. I was initially going to try and provide backward compat, but not anymore

@@ -41,7 +41,7 @@
"url": "https://github.com/davidkpiano/xstate/issues"
},
"peerDependencies": {
"@vue/composition-api": "^0.3.4",
"@vue/composition-api": "^0.6.5",
Copy link
Member

Choose a reason for hiding this comment

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

do we depend on some features in 0.6.5 specifically or could we change this to ^0.6.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can change it. there were bugs caught in 0.6.0->.4 within a few days so didn't want to point to versions that were knowingly buggy but I suppose that's for the consumer to figure out? Was curious why it wasn't ^0.3.0

} from 'xstate';
import { watch, ref } from '@vue/composition-api';
import { watchEffect, ref, Ref } from '@vue/composition-api';
Copy link
Member

Choose a reason for hiding this comment

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

both ref and Ref are unused in this file, could you remove them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea found these test .vue files are hard to type/lint. Will clean them up a bit.

@@ -6,6 +6,7 @@
</template>

<script lang="ts">
import Vue, { PropType } from 'vue'
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, I don't see Vue being used here

Comment on lines 5 to 10
chore(dep/vue) bump peerDep of @vue/composition-api to 0.6.x.

- breaking changes in 0.6.x, adapated code. Will make it easier for
this library to also be updated to Vue 3.
- Uses new watcher option since it was changed to be lazy by default
- now using shallow ref since xstate objects should not be refs deeply.
Copy link
Member

Choose a reason for hiding this comment

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

Prefixes like chore(dep/vue) are not needed, the changelog entry created later because of this changeset file will only appear in the ./packages/xstate-vue/CHANGELOG.md so no need to include this information here. A changeset should also only describe user-facing changes, no need to mention that, for example, a new watcher option has been used internally - it's just an implementation detail.

I propose to change this to something like this:

Suggested change
chore(dep/vue) bump peerDep of @vue/composition-api to 0.6.x.
- breaking changes in 0.6.x, adapated code. Will make it easier for
this library to also be updated to Vue 3.
- Uses new watcher option since it was changed to be lazy by default
- now using shallow ref since xstate objects should not be refs deeply.
Upgraded package for compatibility with the newest `@vue/composition-api@^0.6.0`, which means that a peer dependency requirement has changed to this version, which is a **breaking change**. The only observable behavior change is that exposed refs are now **shallow**.

Which assumes that previously exposed refs were deep, which I don't know if it was true nor what does it actually mean in context of Vue. Would love to learn more if you have a spare moment to explain this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Migrating to 0.6 seems to have changed how ref returns were typed. So actually I think it could work with traditional ref's if the type error could be resolved:

Type 'State<TContext, TEvent, TState>' is not assignable to type '{ value: string; context: UnwrapRef<TContext>; actions: ActionObject<TContext, TEvent>[]; changed?: boolean | undefined; matches: <TSV extends TState["value"]>(value: TSV) => this is TState extends { ...; } ? TState : never; }'.
  Types of property 'context' are incompatible.
    Type 'TContext' is not assignable to type 'UnwrapRef<TContext>'.

^^not sure how to resolve the UnwrapRef<TContext> typing.

A traditional ref will allow for an object's properties and sub-properties to be reactive. The "gotcha" with a shallowRef is that members are not reactive. shallowRef's continue to allow @xstate/vue to work since all ref assignments used are wholesale replaced e.g. current.value = or state.value = (vs. state.value.value =). However, this is a breaking change if users were wanting to mutate return ref's properties directly and still have reactivity like today, e.g. if state.value.value is assigned directly instead of via send, it would not trigger any vue reactivity (watchers, computed, template updates etc.)

Copy link
Member

Choose a reason for hiding this comment

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

A traditional ref will allow for an object's properties and sub-properties to be reactive.

Interesting, is it Proxy-based?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the composition-api is not proxy based, though vue-next (v3) is

Darren Jennings and others added 2 commits June 20, 2020 09:58
@davidkpiano davidkpiano merged commit 97a2ee6 into statelyai:master Jun 20, 2020
@github-actions github-actions bot mentioned this pull request Jun 20, 2020
@darrenjennings darrenjennings deleted the chore/composition-api-bump-0.6.x branch June 20, 2020 22:35
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.

3 participants