Implement direct injection of vue into the constructed instance (Vue 2) #27
Conversation
|
Hi, and thanks for the PR. It looks like you've made some really useful updates! I'm happy to look through the code, but it would useful if you could outline your changes explicitly in your PR intro; perhaps using the changelog format added, changed, removed, etc? Also, thanks very much for the tests. Did you also try your code out with the demos? You can use Also, there is a Vue 3 branch we will need to update. I suggest holding off that until I've had a chance to look at this, and we can go there too. Looking forward to continuing the conversation! Cheers, |
|
I've updated the PR intro with a function-by-function summary of the changes I made. I hadn't tried the demos before, but I just did and SASS is failing because it doesn't support Apple's M1 architecture. I'll try it later on another computer though. |
|
Looking at the Vue 3 architecture, I don't think we'll be able to achieve this same kind of behavior with a decorator. If we want to maintain object identity we'll have to do some superclass shenanigans. Vue 3 can't add reactivity to an existing object because it uses proxies. We could, however, return a proxy from the superclass constructor. There would just be more limitations on the class hierarchy (i.e. only interfaces) |
|
By the way, this feels like a bit of a backwards PR. You're clearly putting a lot of effort in (thank you!) but I'm having to read code to understand what you're trying to accomplish and attempt to work out why and how. Also, it seems like you are aiming to change lots of things / add lots of features in a single PR (not ideal, though appreciate the ambition / passion!). Ideally we would have had:
We're doing:
To help me get up to speed, can you update the PR with a bit of intro?
Or something along those lines. Sorry to be a bit pedantic / obtuse, but I'm mega busy at the moment and getting zero time for OSS, so the more you help me, the more likely I am to merge any changes (rather than ignore this for six months until I have the headspace for it!) In fact - could you start a new issue, and reference this PR? That would be even more useful as I can add labels, reference it from future issues, etc, etc. Maybe copy the format of this: davestewart/alias-hq#17 Thanks Pierce. |
|
No problem! I just created several issues covering the various aspects of this PR, which hopefully help clarify what I'm trying to do here. I don't have much experience with the more formal OSS process, and I got nerdsniped hard by this challenge, so I kinda just dove straight into it. Plus my development style tends to be… haphazard isn't quite the right word, but in the process of fixing one thing other stuff often get swept along for the ride. I get in the mode of "I see problem? I fix problem." and start squashing bugs and fixing deficiencies as I go. The most opinionated change I made, which I just reverted, was excluding |
|
Yay! Thanks 🙏 I had a situation last year with Vuex Pathify where Vue Class Component was added by a contributor. I didn't really get why it was needed in the core, but I went along with it as I was busy at the time, and these were very bright guys making the proposal. Anyway, 2 years later and with the migration it's now causing issues so I'm being more careful now. There's far less code here, so hopefully I can get round to looking at this sooner rather than later 😃 |
thecodewarrior
changed the title
|
Any update on this? |
|
Hey, Had planned to get a bunch of OSS done over Christmas, but just ended up taking a very well earned break. However, I'm taking January out to get a bunch of stuff done, including ticking off all my outstanding OSS tickets. Obviously it takes a fair bit of cognitive switching effort to jump between projects like this, but I am committed to doing it. Don't panic, I have not forgotten you! |
|
Awesome! Glad you had a good break. We've been hosting our build directly off GitHub for the time being (https://github.com/DFStudio/vue-class-store/tree/dfs-dist), so it hasn't been a significant hindrance, but having an official release would of course be better. |
|
Looking at this again I noticed I didn't remove the |
|
Hey @thecodewarrior! Just to let you know I spent this evening cleaning up a bunch of OSS projects. This doesn't necessarily mean I'll have the time to work on everything right now, but at least I can see the wood for the trees. How's things with you? Are you still using Vue Class Store (or your fork of it) in your own projects? |
|
Things are going well! We're using our fork currently, and haven't had any issues with it. |
|
Cool! I'd love to find the time to review everything soon. There's some fairly gnarly code in there. Would a zoom be out of the question if I wanted to get up to speed with things? |
|
Yeah, I'd be up for a zoom call. I'm west coast and have a fairly open schedule most of the time |
|
Cool! Not sure how best to organise. Are you OK with calendly links? Some people hate them! |
|
Yeah, that works! Also it's been two weeks already!? Time really slips away when writing unholy numbers of unit tests. I set up a time, but if it doesn't work for you now we can reschedule. |
|
It works for me! Looking forward to it 🙏 |
|
This branch is ready to merge 👍 |
|
You're a machine! Will take a look tomorrow and report back |
I managed to implement directly injecting Vue into the constructed instance, which fixes #26. I also added unit tests for it, which should improve reliability in the future.
This implements #28, #30, #31, and #33. I'll update this branch as those issues are discussed