thedodd / ybc

A Yew component library based on the Bulma CSS framework.
Apache License 2.0
258 stars 37 forks source link

update ybc to yew 0.19 #25

Closed Follpvosten closed 1 year ago

Follpvosten commented 2 years ago

I'm working on updating the whole codebase to support yew 0.19, while also making the majority of components function components (because simple wrappers like ybc does are a really good usecase for function components). I know that this is close to a complete rewrite, but in my opinion, it's for the best of this crate. I'm open to suggestions for changes.

Currently this is a draft because

  1. I'm not using yew 0.19 in my projects yet, so I can't test it yet (I will be able to soon enough though)
  2. Yew 0.19 has not been released yet, and I don't want to commit a git dependency to the ybc repo. This is also why I didn't tag a release yet. However, if someone has way too much time on their hands, they could already attempt to review it.
  3. There are still open questions:
    • [ ] Yew's input event helpers were deprecated with this release, and there's now multiple ways of handling these; I went for the raw wasm-bindgen variant in the initial draft, which meant adding just one dependency, but we might want to consider other alternatives. Affected components: Everything that uses InputData or ChangeData, which means File, Input, Radio, Select and TextArea.
    • [ ] As the router API changed completely, I've barely touched the router components for now; I didn't make them function components and I also didn't change their behavior, they only wrap the newer yew_router::components::Link component now. The RouterButton component doesn't exist in yew_router anymore, so we might want to a) deprecate the ybc RouterButton or b) implement it differently by taking inspiration from Link; however we could also consider removing the direct yew-router support completely. Affected is everything gated by the router feature, but most importantly RouterButton.
    • [x] ~I've replaced almost all places where a Classes instance was created with a classes! macro invocation. I don't know why it wasn't done like that before and would be happy to change it back if there's a good reason. (I've only avoided it where the logic was complex and I was too tired to think about it reasonably.)~ Looks like this is not gonna be an issue! I might still look into making some macro invocations even shorter though.
    • [x] ~Aggressively converting most components to function components might also not be wanted by the maintainer(s) here, so I'm open to changing some of them back. I've only avoided it for components that store state or connect to agents for now; we could go all the way and make those fn components as well, or we could roll back, I don't really care. However I do think that the vast majority of components in ybc benefits hugely from being turned into a function component because most of them really are just simple wrappers.~ I don't think there's any objections to this.
thedodd commented 2 years ago

Nice! I'm stoked to see this. Just let me know when you want me to give it an eye, and then I'll give it a review.

Follpvosten commented 2 years ago

So far I've verified that the simpler components definitely work with the new code (the simple wrapper components which I have managed to replace with function components for the most part). Input also seems to work pretty well, so that's good; as does the navbar dropdown, and the checkbox (the most complicated one I've actually attempted to turn into an fn component, apart from the various input ones which only get complicated because of the new event handling issue).

Now the problem is that this is the extent of the advanced ybc components I actually use in my app at the moment - I'm considering adapting some of the more complex ones such as File as well, however my project-internal wrappers around file input and similar things all do some special magic, for example accepting drag-drop events in addition to normal use, so I'm not entirely sure how to proceed (if I should move these features into ybc as well or if I can find a way to wrap ybc while also providing additional features on top - I'll see).

I'm considering adding a demo page as a big example to ybc - basically a page mirroring parts of the docs on bulma.io, just implemented with yew and ybc, so we'd have both a nice demo page to show off the framework, as well as a sort of "testing ground" to look out for obvious breakages. Still thinking about whether I can afford the time to actually do that though.

I'm now gonna check of some off the question in my OP that I've already cleared up.

Follpvosten commented 2 years ago

@thedodd so while my code probably is not ready for review yet, I have some higher-level questions that you could maybe answer or give your thoughts about right now (or just tell me that I'm now the maintainer and get to decide myself lol):

On the first unchecked item on my list: I think I've mostly figured this out and it's fairly simple to do by making use of wasm-bindgen, but this does add one more direct dependency in wasm-bindgen. I could switch to the yew-provided version of it, TargetCast, which would reduce the amount of code required, but then the question of how to handle errors is open again (currently I use UnwrapThrowExt from wasm-bindgen to throw a JS error when an invariant is violated). Thoughts on that?

On the router side, I'm wondering about two things: Whether we want to continue to directly support router at all (I think it would still be nice), and if so, how to name our components lol. The Router* components were completely removed from yew-router in favor of just Link<Route, Query>, but a "Link" suffix would kinda conflict with some components in Bulma, so maybe we could still keep the Router prefix. Another question is if we want to keep RouterButton when there's no yew_router RouterButton to wrap; this would likely mean implementing it ourself, which probably wouldn't be too bad. I'd also adjust our APIs to the yew_router ones - they're using to for the route prop now, so I'd adapt the same as it's shorter and very clear.

05storm26 commented 2 years ago

Hi @thedodd, could you merge this?

I have not looked at the code of this change too much but I am using ybc with this change with yew 0.19.3 and it seems to be working quite nice from a library user perspective.

I also can see that there haven't been any update to this repo for 6 months now, is this still an active project?

Follpvosten commented 2 years ago

I'm very sorry for my lack of progress on this. I opened this PR with the intention of writing a full demo page for all YBC components, at a moment where I had a bit of time on my hands to work on it. However soon after I had to get back to my job, where I do use YBC, but as long as everything compiles, the priorities are a bit different. I might get some time this or next month, but nothing is for sure.

Without this demo page, I don't really have an option for testing whether all components work - specifically the more complex ones, like the Router integration stuff and the File component. I'll mark the PR as ready for review. Though it might need some cleanup and testing still, it's working for my purposes at least.

SirVer commented 1 year ago

My 2c: Very interested in seeing this merged. As a first time user of yew it seems that half of the eco system is not on 0.19 and 0.18 seems to be vastly different. As far as I can tell, this library used to be the de-facto standard for components library for yew, but it not being on 0.19 putting it at danger of obsolescence.

Follpvosten commented 1 year ago

As I said in my last comment, I think this is ready to get merged, but I guess @thedodd or someone else would want to review the code first. The last change I pushed raises the MSRV to 1.62 btw.

half of the eco system is not on 0.19 and 0.18 seems to be vastly different

Yeah - I think the catch here is that 0.19 reduced the boilerplate for defining components a lot. Just looking at the numbers for this PR; it removes about 2.5k loc while replacing them with less than 1k equivalent lines. As an example, look at the diff for the Columns and Column components: https://github.com/thedodd/ybc/pull/25/files#diff-63f3ac37f73736def1c2a6e6355eee325ab4d71c95216a7cacc6cadc5ffd3d73

At the same time, Yew 0.19 didn't add a way to "pass down the rest of the props to a child node". This is being tracked here https://github.com/yewstack/yew/issues/1533 but nothing is really happening there.

In reality the combination of these circumstances means that component libraries are now much less useful than they could be - if the upstream library forgot to expose an event handler you need, you have to either add a wrapper and hope that it bubbles up, or open an issue/PR on the upstream. With how easy it is to define your own component, I suspect this effort is often just not worth it, and people end up just defining their own component which exposes the functionality they need. I know I do.

Follpvosten commented 1 year ago

As to #27 - this is something that's not guaranteed for this branch either. Once this PR is merged, it would likely be a good idea to make this repo's master branch target the yew master branch.

Follpvosten commented 1 year ago

During development of my demo page I noticed that using Option<Classes> on the one hand doesn't improve memory usage or performance in any way (since an empty Classes still doesn't need allocations), and on the other hand makes the API of the components a lot worse (&'static str implements IntoPropValue<Classes>, but not IntoPropValue<Option<Classes>>, which means you'd have to do an explicit classes={classes!("is-primary")} everywhere even for simple cases like that; using Classes makes that as simple as classes="is-primary").

The only downside of this change is that internally we now have to do explicit clones, but from an API and performance perspective it is superior in every way.

SionoiS commented 1 year ago

Yew v0.20 is out.

Seams like the logic for modals will need an update to use the new Context API.

@Follpvosten do you plan to work on this? If not I might.

thedodd commented 1 year ago

@Follpvosten I don't have time right now to do a full review, but what I think we could do is cut a beta release! That will give us a chance to merge, and folks can use and experiment if they are so inclined.

@SionoiS yea, for the 0.20 ... I could merge this PR, and then you could open a new PR with the 0.20 updates. What do you think? We could just make that the next alpha.

Follpvosten commented 1 year ago

@SionoiS right - they deprecated non-worker agents, so you will have to build an API on top of ContextProvider and channels instead. I'm afraid I once again don't really have time to work on this right now, and sadly it's also unlikely that our internal codebase is gonna use 0.20 anytime soon, so go ahead, I won't have time to do this.

@thedodd merging and cutting a beta release does sound reasonable. After that I could open a separate PR where I add my demo page, so other people (or me when I can find the time) can complete it at some point.

SionoiS commented 1 year ago

Merge this as beta then v0.20 in separate PR, sounds good to me!