-
Notifications
You must be signed in to change notification settings - Fork 43
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
Using Avivator as a library #745
Comments
Excuse my less-than-stellar git skills - I merged recent changes where I should probably have rebased, so there is currently a lot more noise than I'd like in my draft PR. Hopefully I'll manage to make that a bit cleaner. |
We will have a discussion about this. We have been not in favor of this in the past for the reasons you alluded to (more docs, responsibility, etc.) but perhaps there is some middle ground. Thanks! |
I hope there's a middle-ground, because I think it'll be a bit of a nuisance for us otherwise I'm afraid. I appreciate that when there is only one context this ends up making the code marginally less clean, and it's unfortunate that it ends up leaking into wider parts of the codebase... the main problem with that as far as I'm concerned is when it comes to merging your upstream changes. Great to see some active development, but really hoping not to have to keep doing this... There are a few things I want to potentially change later with shaders etc that may hopefully be of some benefit to you upstream... but I may end up having to make an Avivator fork and none of us want that. Shader things are probably mostly at the Viv layer (although I have some experimental shader editing Avivator UI in another branch - not in a state that I would suggest would be suitable for the main app at all, although I may want to develop it further into something that could be made more widely useful). Anyway, thanks for your response. I could be willing to contribute in terms of documentation if it helps - not sure how bad it would be though to just have a heavy disclaimer on this not being something you want to support, in which case I don't think it'd make for any particular extra maintenance on your part. |
I think shader changes are much more likely to be accepted since I am the one who has responsibility over them. But making a changing like promising some API to end-users would be a lot for Avivator. What is your plan beyond the context business for Avivator? Is that all you would need? |
Well, what I want to be able to do is Anyway, so in concrete terms I'd want to be able to pull in a As far as avivator-as-a-library is concerned, I'm not worried about it being documented or supported (although hope that if I do run into trouble you'll be able to help - generally I can find my way around ok)... so as I say, I hope that it will be possible to expose what I need with appropriate heavy disclaimers associated and no particular cost in terms of maintenance. I could potentially make a fork keeping changes to a minimum beyond whatever is needed to expose as a library, it's just unfortunate that the way Zustand works means that the context related changes leak across the code-base making that a chore to keep up-to-date. |
Sorry for my limited availability to address this PR right now. The end of the year push is pretty intense for me, and I'm hoping I'll be able to have a look in a couple of weeks to think about this more deeply. I am very open to the PR for using the context to make the use of Avivator in your use case.
While I understand this would be ideal for your usage, I don't think we as a team have the capacity to maintain Avivator as a library. I hope that we can find a middle-ground where you are able to find a way to remix pieces of Avivator in your code (hopefully your context PR helps with that) but we are at capacity maintaining Viv's core for library consumers. Exposing Avivator as a package on NPM would be a massage API surface area that we would be taking on long term. There are choices with dependencies in Avivator that we would likely would have made differently if we intended it as a package. |
A nice middle ground could be doing things that make installing directly from git easier via npm in some way. I'm not sure what this could look like, but even if it's not encouraged, PRs like the one you already made would be welcome as long as they don't mean taking on an API. In other words, if you want to get yourself familiar with the Avivator codebase, you're welcome to do that but we can't promise anything about it (besides that the demo will work). |
That all seems perfectly reasonable. I'm pretty familiar with the Avivator codebase already, and don't want to incur extra maintenance burden on you... admittedly my own experience of maintaining modules that are actively used more widely is pretty limited/non-existent. Hopefully we'll be able to sort something out that allows for the kind of use I want without putting you on the hook, but I can see there are reasonable grounds for caution. Interesting point about dependencies in Avivator that I had somewhat pushed out of mind... appreciate that these things can be easy to overlook from outside but then end up being the thin end of a metaphorical wedge... I'm sure it's not the only thing, but I have mixed feelings about Material-UI before, and did vaguely notice the versions you were using being no-longer supported... I can see that being a root of potentially bigger and longer-term headaches than me just doing what I was originally going to do, treating Avivator more as a 'demo' as you say... it may be that there's not that much point in directly re-using much of your UI code in the end, and the state stuff is relatively small & perhaps not worth all this fuss. I just started to get the feeling that with development suddenly starting to be very active on your side, it could be a bit of a hassle to keep doing the same refactors... So I think there's still some benefit to the context changes - but to be fair there's also an argument that for Avivator itself, they do add a bit of noise to the code... Anyway, best wishes @manzt & @ilan-gold ... sorry if I'm rambling etc. |
User story
In MDV, we support loading arbitrary sets of charts, including Viv based views. I'm gradually refactoring some of our code (particularly Viv-related parts), moving more towards React and it'd be nice to be able to re-use some Avivator components, as well as the hooks and state.
Preferred solution
Zustand stores can be adapted to use React Context API. In Avivator, there can be one provider for the whole app which doesn't appear to impact on functionality AFAICT.
I posted this a couple of weeks ago, and will submit a PR referencing this issue based on my branch.
Possible alternatives
I really don't want to have to keep maintaining a fork, or re-implement my own version of equivalent code.
Unfortunately I don't think there's a way to do this that doesn't touch all parts of the code where
useXxxStore.setState()
is used, but would be happy if someone could correct me.The text was updated successfully, but these errors were encountered: