-
-
Notifications
You must be signed in to change notification settings - Fork 834
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
Rewrite JavaScript component layer for Mithril 1.0 #872
Comments
Btw if we go ahead with this, we would do it in an incremental non-breaking way. So we can convert one component at a time without the deadline of any particular beta release, and then deprecate the old way, and then remove it before stable. |
Also we could ditch the React-like API idea and just make some refinements to the current API, moving as far as we can to the other end of the spectrum (familiarity with Mithril's API). I'll brainstorm for that possibility in the morning. I guess the most important thing is to extract it into an external package and document it properly. |
As mentioned in the previous comment, I wanted to brainstorm going the other way (familiarity with Mithril's API rather than React's). Today I did a bunch of experimentation and research, including looking into the APIs if other virtual-dom frameworks, and trying to deduce a bit about what Mithril 0.3 and 1.0 will be like. Here's what I've come up with: class Counter extends Component {
constructor() {
// 2. Set initial state
// Stick with the freedom of the Mithril approach here, and encourage
// use of m.prop.
this.count = m.prop(0);
// 3. Determine whether the component should redraw by dirty-checking a value
this.observe(this.count);
}
// Named `render` rather than `view` because the superclass needs to
// do some stuff to the subclass vdom, but we want to keep the external
// API consistent with Mithril
render({className, icon}) {
// 1. Initialize props as they come in
// No need for a separate method, doing it in the view is fine
icon = icon || 'mail';
return (
<div className={className}>
<i className={'fa fa-'+icon} />
// 4. Get and set component state
<input type="text" value={this.email()} onchange={m.withAttr('value', this.email)} />
</div>
);
}
// Split up `config` so that subclasses and extensions can more easily
// override/patch these individual hooks
didCreate(context) {
// 5. Initialize the DOM
}
didUpdate(context) {
// 6. Update the DOM
}
didRemove(context) {
// 8. Destroy the DOM
}
// Wrap the context.retain API to make it more accessible
shouldPersistDOM() {
return true;
}
// Wrap the onunload(e) API to make it a bit nicer, and extensible
shouldPreventUnload() {
// 7. Hook onto component dismount, and potentially prevent it
}
} This is a relatively minor change to what we have now, compared to the idea of going "full React". So it shouldn't be too hard to implement :) I'll name the package |
Looking good. 👍 Just a quick thought: If we went for a fully React-like API, would that give us compatibility (i.e. using React components)? Not if I understand it correctly, right? I suppose the underlying VDOM implementations wouldn't be compatible... Regarding the transition: We could first create and fully test the package, before starting to transition the core components (and then the extensions), and support both APIs for one or two beta releases. (Given that they just extend Mithril, they will be compatible anyway, right?) Do you want to host this under your account or under Flarum's? |
Correct. We're still able to use any other Mithril components though, no matter what API we choose to implement.
Sounds good.
My account. I think we should keep the Flarum organisation for Flarum-specific stuff (extensions, Flarum helpers/utils, docs, etc.) Also, I forgot to mention a little perk: more than likely, this should future-proof us against any breaking changes in Mithril 0.3/1.0 :) |
How so? Because we abstract it? |
Yep. So any breaking changes we can just update in our abstraction. |
Some other JS API todos while I think of them (will create more issues later):
|
Probably won't need to write this library as it looks like Mithril 1.0 will be coming soon – and it includes everything we need out of the box! |
inferno is another library to think about, I know it's lots of work but it seems to be more promising and faster and also smarter. |
I always wanted to contribute to this project but having Mithril instead of React makes it difficult for many of us who are more familiar with the latter and don't have the time to learn the former. |
Because react is slow. Mithril is extremely Fast and lightweight. That's why it was chosen. Inferno may be a good candidate. |
We'll be moving to Mithril 1.0 at some point in the future, which has a better API so we don't need to try and imitate React's. Some of the improvements are inspired by Inferno, so I don't believe Inferno offers any significant advantages over it. |
@dav-is Do you have any test which shows that some part of Flarum offers a better user experience with Mithrill rather than React? I know the benchmarks comparing Mithril (and other virtual dom libraries) vs React but in a real-world usage React is just very fast and surely fast enough for users if used properly. For instance Netflix is using it and they support devices 256 times slower than a common laptop. I think the difference between React and Inferno, Mithrill and others like preact is that React is battle tested and covers a lot of edge cases. It also has a very large community and tooling. It offers a much better developer experience and helps building solid products in my opinion. I'm not saying that React is more suitable than Mithril for Flarum, @tobscure is the person who knows best the requirements of the project. I'm just curious about the reasons behind the choice, apart from being "faster" in benchmarks rendering 10k rows. As I said before, you are loosing some contributors because of this choice and I would like to know what reasons make it a good compromise. I'm not here to criticize but to help and maybe learn. |
I think that migrating to React isn't anywhere in scope for the upcoming major release. ;) |
@ezeperez26 Maybe you should read more carefully the patent.
As many Facebook interns have already confirmed, this applies only when you are initiating legal action against Facebook. I seriously doubt Flarum will ever legally issue Facebook for something. The article title is just click-bait. You can read more about this topic from official voices here: facebook/react#7293 |
For reference: This ticket can hopefully also be closed once we upgrade to Mithril 1.0. |
(Mithril core dev here) Just chiming in to ask: what issues have you all had with migrating? In particular, is there anything that could be done on our end (Mithril core or related in this project of mine) that could help ease migration? |
Hey @isiahmeadows! We haven't attempted migration yet. Thanks for the link - that should make it easier. I'll give it a try soon. Is there an equivalent of |
For that, we've mostly replaced it with an `onbeforeupdate` lifecycle hook,
which functions similarly to React's `shouldComponentUpdate`.
…On Thu, Feb 8, 2018, 15:59 Toby Zerner ***@***.***> wrote:
Hey @isiahmeadows <https://github.com/isiahmeadows>! We haven't attempted
migration yet. Thanks for the link - that should make it easier. I'll give
it a try soon.
Is there an equivalent of {subtree: 'retain'} in Mithril 1.0?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#872 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AERrBBIAaM-bjldtlM98aci-hcefyptAks5tS2A6gaJpZM4HzsjR>
.
|
@edittler – not the case anymore now that it's under MIT license.
@jiayihu – who knows how popular Flarum would have been now had they went with React or Vue.js with their vast ecosystem and network effect. But it's their choice to stick with Mithril (and the occasional sprinkle of jQuery). |
@isiahmeadows Thanks for getting back! We discussed this in our latest developer meeting and decided we would wait for and then jump directly to Mithril v2. |
Made class list for post extensible by using a separate method.
@isiahmeadows Hello, not sure if you'll see this but I'll put it here nonetheless. I am currently in the process of upgrading Flarum's JS frontned to Mithril v2.x, and have run into some issues that I'm not sure how to resolve. If you could provide any insight and/or recommendations on how things should be connected, that'd be greatly appreciated. I've looked through new and issues, documentation and SO questions, but haven't been able to figure it out. You can check out the latest code at https://github.com/flarum/core/tree/ds/frontend-framework-rewrite-mithril/js/src. Thank you for your time.
|
I plan to come back to this later with a more in-depth analysis, but Mithril v1 and v2 have the same component interface - the only major difference is that you can't do Regarding Also, your inline component will always fail anyways, because you're creating a new component each time, and Mithril diffs that via a simple |
@isiahmeadows Thank you for your response, I'll try what you suggested. As for the The main point is that we want to modify the component state by using its methods from outside the component. Right now, the saved component instance in EDIT: When I said that
Yeah, that's why it didn't work as a solution. Not sure how |
@datitisev You can pass lifecycle hooks in Regarding the stream issue, consider passing the component state instead, and consider not cloning the result, or at least avoiding cloning the element. If that's not possible for whatever reason (say, it's going through a worker boundary or similar), you might be able to get away with an ID pool not unlike what I did to coordinate arbitrary requests across an IPC channel, and just using an ID → value map where you set it at the sending end and read at the receiving end. You just need to be sure to release the ID once you no longer need it. (It's conceptually very similar to C's |
I decided to look a little deeper, and found something highly suspect. Right here: this is only a small variation of what's described in this anti-pattern in the docs. You need to have components return their views directly, and as long as you use the component subclasses directly via For your PostStream specifically, if you want a surgical fix, try setting your scroll listener directly in But in either case, you'll find it a lot easier to manage if you try to transition into a system where you're doing |
@isiahmeadows Thank you very much for your thorough response. I'll see what I can do with the information you've given us here and try to implement some of your suggestions. I really appreciate it. |
Welcome! |
@isiahmeadows Sorry to bother, I'm not the strongest technically speaking and I'm having a hard time understanding the meaning of this:
How would you pass the component state and what suggestion with using ID's do you mean? Could you potentially clarify this (with a snippet or linked to code)? Thank you and sorry to be a pain, your help has been exceptional ❤️ |
Where you would ordinarily pass the element ( |
And the IDs part, you can ignore that bit unless you genuinely can't pass the component instance anywhere, something usually only due to technical restrictions (like you're passing data through a web worker and back or similar). In my case, I had to work around an OS-level process barrier where shared memory doesn't exist at all leaving cloning the only way in theory, so I had to do something much more complicated to work around it. The goal was to call a function and get a result, but that module of mine fundamentally has more in common with an HTTP implementation than, say, Relay or whatever. This was a particularly advanced need, though, one I've rarely encountered anywhere else. |
Haha, that's not going to apply for us, thankfully. @isiahmeadows Thanks for the awesome support and for Mithril! ❤️ |
@isiahmeadows Not sure if I implemented it correctly. It kind-of works, the only problem is that the element does not redraw at all (onupdate and onbeforeupdate hooks aren't even fired) on the component (in this case PostStream) or its children. The DOM doesn't recreate now, which is good, but for some reason redrawing doesn't occur at all? I've been debugging, trying to figure out what the root cause is, and it just seems to be my implementation of what you've suggested. The children redraw fine when outside of the PostStream, which means the issue is with how PostStream is rendered in DiscussionPage. You can view the changes @ f39d0ab#diff-ccd768950e2518aaa4b440287d036a07. And again, thank you for all your help so far. |
Your issue: don't retain vnodes unless you really absolutely need to. If
you return the same vnode as before, it's like you returned `false` from
`onbeforeupdate`. Don't store them, just read them as necessary. It's why I
recommended just storing the state, not the entire vnode.
|
@isiahmeadows Oh I see, my apologies, O think I misread it. How would you recommend we store it? I can only think of adding it to an oncreate hook on the vnode attrs for the component as as far as I'm aware, the state is not set before then? |
`this` is your component state, and `vnode.state` is set to it. It's set
the moment you're in a lifecycle hook. And `oninit` is the lifecycle method
you're looking for - it's set *before* the view is rendered. `oncreate` is
called *after* it's rendered, and that's when you store the DOM node.
You can find more details here:
https://mithril.js.org/lifecycle-methods.html
On Tue, Mar 10, 2020 at 03:01 David Sevilla Martín ***@***.***> wrote:
@isiahmeadows <https://github.com/isiahmeadows> Oh I see, my apologies, O
think I misread it. How would you recommend we store it? I can only think
of adding it to an oncreate hook on the vnode attrs for the component as as
far as I'm aware, the state is not set before then?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#872>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABCGWBD6HSFGOLVB7RINRULRGYFZBANCNFSM4B6OZDIQ>
.
--
-----
Isiah Meadows
[email protected]
www.isiahmeadows.com
|
Ah, I see. Thank you for all your time and help. |
Considering the changes we're making in mithril 2.0, how much of this is still relevant @datitisev? IMO, with the new component lifecycle methods, the only part that might make sense to keep (although probably in an altered form) is:
|
@askvortsov1 Not sure. |
The still relevent bits of this have been extracted out; the rest is no longer relevant as there's no clear direction here. |
Part of #262.
The base Component class is an abstraction layer on top of Mithril's raw components which makes components a bit easier to work with, a bit more React-like, and is better for extensibility.
It's very generic, with no specific ties to Flarum, so it should be extracted into its own external package so it can be used in other projects and developed independently of Flarum.
Unfortunately, its API is also very unfamiliar. It's really an odd mix of Mithril and React. We have the
view
method from Mithril,config
as a method instead of a vdom attribute,onunload
from Mithril,this.props
from React,this.$()
from Ember, and some of our own inventions likeinit
andinitProps
. This mixture is bad for onboarding new core/extension developers, because they'll have to learn something new/different even if they're already familiar with Mithril/React.I propose that when we extract this package, we change the API so that it reflects React as closely as possible. (The React API is more flexible and better for extensibility than the Mithril API.) The package can be called
tobscure/mithreact
– a React-like API for Mithril components.Here is an example of a simple component – first with the current API, and then the equivalent with my proposed Mithreact API:
Current
Mithreact
Advantages of Mithreact:
Disadvantages:
Thoughts?
The text was updated successfully, but these errors were encountered: