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

Add Vue 3 support #203

Closed
wants to merge 1 commit into from
Closed

Conversation

axwalker
Copy link

@axwalker axwalker commented Jul 27, 2020

This is very much baby steps at the moment. The general approach is:

  1. Remove all vue 2 specific libraries
  2. Install vue 3 equivalents
  3. Change implementation/tests throughout until all existing tests pass
    a. I've started this by working through the tests in FormulateForm.test.js one by one
  4. Extract testing environment for Vue 3
    a. remove vue 3 libraries from main build into separate directory or similar
    b. create separate directory for installing/running vue 3 tests
  5. Reintroduce vue 2 libraries in main code
  6. Reintroduce original tests for Vue 2
  7. Update implementation and tests again to be compatible with both Vue 2 and Vue 3, until all tests pass on both Vue 2 and Vue 3

Getting everything working in Vue 3 first is a little easier for me coming fresh to the codebase. It's harder at this stage for me to quickly identify idiomatic ways of supporting Vue2/3 simultaneously.

So far I'm at the very, very early stages of 3 - I'll some comments inline on things I've learned so far.

@@ -32,7 +32,7 @@ export default {
name: 'FormulateForm',
model: {
prop: 'formulateValue',
event: 'input'
event: 'update:modelValue'
Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I suppose we'll need to manually emit input events now too for backwards compatibility

// For some reason formulateRegister always returns undefined if given a default,
// even when it really should have been provided
formulateRegister: { default: () => undefined },
formulateRegister: 'formulateRegister',
Copy link
Author

Choose a reason for hiding this comment

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

This felt very much like a bug, but I'd have to check the provide/inject stuff in Vue 3 in isolation a bit more to check if anything has changed here in their API. A glance through their documentation doesn't suggest this should have changed.

return this.value
} else if (has(this.$options.propsData, 'formulateValue')) {
return this.modelValue || true
} else if (has(this.$props, 'modelValue') && classification !== 'box') {
Copy link
Author

Choose a reason for hiding this comment

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

this.$options.propsData is no longer a thing. Now there's this.$props, but I don't think it behaves in the same way - as I stated in a question on the Vue discord:

I'm trying to determine whether props were passed to a child component.

Let's say I have 2 props:

props: {
  foo: {
    type: String,
    default: '', 
  },
  bar: {
    type: String,
    default: '', 
  }
}

And in a consuming component only one prop is passed in:

<my-component foo="someValue" />

In Vue 2, I could determine which props are passed in with .$options.propsData which would give an object containing only { foo: 'someValue'. However, I'm not sure how to access this same information in Vue 3. I know I have .$props, but it seems that also contains bar with its default value.

This means I'm unable to differentiate between a consuming component explicitly passing bar="" and bar not being passed at all.

This is causing some difficulties in helping to port a library from Vue 2 to Vue 3. Any help would be much appreciated!

To which I received the following response from somebody:

@axwdev I'd consider this a pretty big anti-pattern, and I'd very much recommend you find a different way of achieving what you want.
However, you can still get this data.
If you can find the component vnode, it's present in the props property there.
You can access the vnode from this.$.vnode or getCurrentInstance().vnode.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah in my experience there are a lot of things you have to do as a library author that are not exactly accepted common practice. This is one of them for sure.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, the wider the use cases you have to support the more likely you're going have to do unconventional things to accommodate them all!

@@ -330,7 +330,7 @@ function hasValue () {
* Determines if this formulate element is v-modeled or not.
*/
function isVmodeled () {
return !!(this.$options.propsData.hasOwnProperty('formulateValue') &&
return !!(this.$options.props.hasOwnProperty('formulateValue') &&
Copy link
Author

Choose a reason for hiding this comment

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

This should be $props, but it will have the same issue as it does elsewhere.

@@ -438,7 +438,8 @@ function blurHandler () {
* Bound listeners.
*/
function listeners () {
const { input, ...listeners } = this.$listeners
const { "update:modelValue": input, ...listeners } = this.$attrs
Copy link
Author

Choose a reason for hiding this comment

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

$listeners is no longer a thing, so this sort of thing will need to be done throughout. And instead of just $this.$attrs I think we need to pull out only those attributes that start with on.

},
}
return originalMount(app, { ...options, ...withPlugin } )
}
Copy link
Author

Choose a reason for hiding this comment

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

Plugins are now passed into tests as part of the global option in mount. This is get around the whole createLocalVue issue. A few things have been changed in this area.

const wrapper = mount(FormulateForm, {
slots: {
default: "<button type='submit' />"
}
})
const spy = jest.spyOn(wrapper.vm, 'formSubmitted')
wrapper.find('form').trigger('submit')
await wrapper.find('form').trigger('submit')
Copy link
Author

Choose a reason for hiding this comment

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

These functions now return promises.

@@ -43,7 +51,8 @@ describe('FormulateForm', () => {
expect(wrapper.vm.registry.keys()).toEqual(['subinput1', 'subinput2'])
})

it('deregisters a subcomponents', async () => {
// Vue-test-utils no longer provides `setData`
Copy link
Author

Choose a reason for hiding this comment

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

vue-test-utils is more strongly recommending the avoidance of testing implementation details. Them removing setData is part of this.

const wrapper = mount(FormulateForm, {
propsData: { formulateValue: { testinput: 'has initial value' } },
props: { formulateValue: { testinput: 'has initial value' } },
Copy link
Author

Choose a reason for hiding this comment

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

props has changed to propsData in vue-test-utils also.

@axwalker
Copy link
Author

#198

@justin-schroeder
Copy link
Member

Wooooow @axwalker this is really great work. Thanks for taking the time to do this. Very helpful to be able to read through your suggestions and comments here. Much appreciated. Unfortunately I'm starting to see why so many authors have opted for the -next repos.

@axwalker
Copy link
Author

Yes, I had the exact same feeling regarding the -next repos as I was doing it!

@fseemann
Copy link

What is the state of this PR? When can we expect Vue 3 support? Thanks for the awesome library and effort.

@axwalker
Copy link
Author

axwalker commented Oct 21, 2020

What is the state of this PR? When can we expect Vue 3 support? Thanks for the awesome library and effort.

@fseemann This PR probably barely scratches the surface on the changes that need to be made. So currently it's quite far off unless Justin and co have been working on it behind the scenes.

I actually have the next 2 days free so I could take another proper crack at this - instead of the hour or two I spent on this PR! The plan would just be to make everything exclusively Vue 3 compatible for now with no Vue 2 support. Then we can work out what we want to do after that in terms of -next vs making changes to allow it to support both in one package.

@justin-schroeder let me know if you'd already been working on this elsewhere or had any newer ideas on the approach. Otherwise I'll give it a go this week.

@justin-schroeder
Copy link
Member

@axwalker Yeah, I have a somewhat similar plan too, the idea would be to create a vue 3 compatible version on the same feature set. Initially juts as a branch of this project, and probably published with a -next. Eventually, when a full re-write comes down the pipe, the library will likely be re-written with typescript and exclusively for vue 3 (maybe demi for backwards compat?). That version would be master on this repo and tagged as version 3.

If you're planning on spending some time working on Vue 3 compat that would be awesome! I'd suggest trying to get the test suite to run as one of the first orders of operation and then starting to backfill functionality based off those. To @fseemann's question — the time horizon is a little fuzzy right now, it is looking like ill have some time to seriously dedicate to the effort in the ~1 month range, but as with any OSS its hard to estimate how far and fast that time block will move this. My goal is to have Vue 3 compatibility ages ago.

@axwalker
Copy link
Author

@justin-schroeder I've spent a couple more hours today on this but I'm feeling like my time on it is just going to be 20x less efficient than yours!

I started a new branch on my fork from the beginning cause Vue/your library have gone through a couple of minor changes so felt it was easiest that way. Also made it easier to refresh my memory doing it again, too.

So far just have the following tests passing:

Screenshot 2020-10-22 at 16 17 34

But every other new test I unskip fails and it takes a fairly long time for me to understood exactly how that piece of functionality is actually working before I make the necessary changes. Not sure how valuable it is for me to continue at this stage but hopefully there's something useful for you there!

@axwalker axwalker closed this Oct 27, 2020
@axwalker
Copy link
Author

See #295 instead.

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