Skip to content

Conversation

@MichaelFedora
Copy link
Contributor

@MichaelFedora MichaelFedora commented May 27, 2022

I was able to make it compatible with both versions, except for typings. I don't know how to run your tests / they were not able to be run, but it theoretically should work. If you could run them that would be great!

I'm also not entirely sure the typings will work but they should.


Changes

  • add typings for vue3
  • move computed items to data just be assigned in mounted() because they don't change and don't need reactivity
  • expose recalculate
  • add props to be more vue-y
  • add type checking
  • add docs to props
  • update package.json for vue3 whatnot
  • (forgot to add to the commit) scroll just directly $emits instead of a weird pass-through
  • get attributes via $attrs instead of $refs.element.attributes or $el.attributes

Supersedes #578 , closes #557, #600, and #588.

- add typings for vue3
- move computed items to data because they don't change
- expose recalculate
- add props to be more vue-y
- add type checking
- add docs to props
- update package.json for vue3 whatnot
- remove excess methods
@renatodeleao
Copy link
Contributor

renatodeleao commented Jul 28, 2022

@Grsmto any reason to hold or not reviewing this? I was about to make a PR myself, but this covers it and add some extra good stuff namely the props API.

@MichaelFedora if you allow me to review, the only things I would tweak:

  • Remove initialisation of the non-reactive variables in data, doing them directly at mounted() is enough and that way prevents vue to attach reactive handlers to the objects. It's a minor perf thing
  • remove the installation of vue3 and friends as devDependencies, that's why tests are failing: if you want to make tests work in vue3, there's more stuff to do as @vue/test-utils have a few breaking changes
  • Since this is supposed to be cross-compatible, if we want to have both vue2 and 3 tests at the same time there's a bit of work required to make it happen: installing vue3 packages with npm alias, have a swap-script like vuelidate, etc. --> check out this PR for "inspiration" if i may suggest.

- also add descriptive type comments
@MichaelFedora
Copy link
Contributor Author

MichaelFedora commented Jul 29, 2022

Hey @renatodeleao ! I honestly have no idea what I'm doing with the testing libraries (it's my weakest skill), I simply copied some stuff from #578 . If you want to fork my branch and make your own PR I'll gladly close in favor of yours?

I adjusted the reactive data definitions and added some more type comments. I've tested in Vue2 by using this exact element in a professional project (along with adding import 'simplebar/dist/simplebar.min.css'; to it), but that's all I have.

@Grsmto
Copy link
Owner

Grsmto commented Jul 30, 2022

@Grsmto any reason to hold or not reviewing this?

Hey @renatodeleao I'm currently on vacations but I'll be able to review this soon. I would be happy to let you review it since I'm not using Vue at all I'm probably not the best person to do it anyway!

@renatodeleao
Copy link
Contributor

@Grsmto no problem, enjoy your vacations! 🏖️

I'm a bit short on time, but I'll try to squeeze this into the work agenda.

@anburocky3
Copy link

@renatodeleao Any update on this?

@MichaelFedora
Copy link
Contributor Author

@anburocky3 if you do need this ASAP I do recommend just copying the file and using it locally, it's what I've been doing for the past few months.

@anburocky3
Copy link

@anburocky3 if you do need this ASAP I do recommend just copying the file and using it locally, it's what I've been doing for the past few months.

It is using vue2 right? I'm looking for latest vue3.

@MichaelFedora
Copy link
Contributor Author

It should work in both.

i could not setup the project using latest node 16, sharp package failed to build because of some native bindings. Not sure if related with my m1, but though i would add this. Tested with 12 and 14 and installed fine.
We can now run same tests as vue 2.6, 2.7 (composition-apo) and 3.x from a single file test file. Note we still need to adjust the mountingOptions signature because vue-test-utils has some breaking changes from 1 to 2. If tests get better (more complext) an isomorphic wrapper around mount/shallowMount should be added to test-utils/.js.

- adds swap-vue.js script and vue-demi: this is straight from vuelidate codebase
- adds aliased vue versions of core, compilers and vue-jest. swap-vue and vue demi will set the proper versions beforehand
- using isVue3 flag from vue-demi we conditionally re-export @vue-test/utils from the appropriate version
- called html() before snapshots, because jest-serializer-vue is not yet compatible with vue3 eddyerburgh/jest-serializer-vue#56
-
- it's a breaking change to remove it, because $refs are exposed and consumers might be expecting its presence
- this.$attrs in vue3 contains class, style and event listeners altogether. Also, Simplebar.getOptions only works it NamedNodeMap not a plain object like $attrs: and now that tests are working you could actually see it failing before this fix.
Simplebar was not unMount when component is destroyed, which in conditionally loading scenarios potentially could lead to memory/event leaks.
@renatodeleao
Copy link
Contributor

renatodeleao commented Nov 17, 2022

Folks update: I've patched this PR with isomorphic testing and a couple of fixes to make this work on both vue versions!
MichaelFedora#1

I'm sorry for the delay and the kind of ghosting but life happened and I had a baby 🎉... so didn't have much free time for OSS... neither have now but got some employer sponsor time for this so here it is!

@MichaelFedora didn't push directly to your fork because don't have write access but merge the PR when you can. Then you might have to sync this with master and hopefully nothing will break 🤞 — I had some trouble setting up the project because of node versions and some existent dependencies.

Cheers everyone!

renatodeleao and others added 4 commits November 17, 2022 15:08
and original 'test' to 'jest', this way we keep ci happy, since it's expecting this name
it's not necessary, i've added while bashing my head against the wall trying to make this work
feat: isomorphic testing for vue 2 and 3 and relevant fixes
@MichaelFedora
Copy link
Contributor Author

MichaelFedora commented Nov 21, 2022

Merged and congratulations @renatodeleao ! 🎉

The yarn.lock might be screwed up but otherwise I was able to get it up-to-date. Might have to abandon testing for it otherwise, as it's such a simple wrapper 😬 oh well. Travis CI seems to have liked it though.

@Grsmto
Copy link
Owner

Grsmto commented Nov 22, 2022

Thanks for the work on this! Let me know when you think this is ready for merge and I will happily do it.

@renatodeleao
Copy link
Contributor

renatodeleao commented Nov 24, 2022

@MichaelFedora thanks for the merge!

The yarn.lock might be screwed up but otherwise I was able to get it up-to-date.

Fetching your branch, and running yarn does update yarn.lock, so when you have the time please run it and then commit/push the changes to the branch so that @Grsmto can finally merge this.

When you're at it, one last favour:

"engines": {
"node" : "<16.0.0"
},

remove these 3 lines, after the master sync they're no longer necessary.

Might have to abandon testing for it

Why? Testing is running fine locally and on CI.

- fix missing semicolons and set SimpleBar to undefined instead of null
- update yarn.lock
- remove "engines" from package.json
@MichaelFedora
Copy link
Contributor Author

MichaelFedora commented Nov 25, 2022

@renatodeleao

Fetching your branch, and running yarn does update yarn.lock, so when you have the time please run it

I was trying to avoid this with the 18k dependencies but, done 😅

remove these 3 lines, after the master sync they're no longer necessary.

Done!

Why? Testing is running fine locally and on CI.

Yeah I was saying this if it kept failing, but seems to be fine!

@MichaelFedora
Copy link
Contributor Author

@Grsmto Seems we're good to go!

@Grsmto Grsmto merged commit b567f41 into Grsmto:master Nov 27, 2022
Grsmto added a commit that referenced this pull request Nov 27, 2022
@MichaelFedora MichaelFedora deleted the vue-3 branch November 27, 2022 18:34
@anburocky3
Copy link

@Grsmto When will next branch merge into main branch?

@Grsmto
Copy link
Owner

Grsmto commented Nov 27, 2022

This has been release in simplebar-vue@1.7.0.

Grsmto added a commit that referenced this pull request Nov 28, 2022
@MichaelFedora
Copy link
Contributor Author

Did something break that caused this to be reverted?

@renatodeleao
Copy link
Contributor

Yup, see #630 which is another attempt.

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.

Simplebar + Vue3

4 participants