Rewrite JavaScript component layer for Mithril 1.0

tobyzerner commented 8 years ago

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 like init and initProps. 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

class MyComponent extends Component {
  // 1. Initialize props as they come in
  static initProps(props) {
    props.icon = props.icon || 'mail';
    props.className += ' bar';
  }

  init() {
    super.init();

    // 2. Set initial state
    this.email = m.prop('toby@flarum.org');

    // 3. Determine whether the component should redraw by dirty-checking a value
    this.subtree = new SubtreeRetainer(this.email);
  }

  view() {
    // 3. Determine whether the component should redraw by dirty-checking a value
    return this.subtree.retain() || () =>
      <div className={this.props.className}>
        <i className={'fa fa-'+this.props.icon}/>
        // 4. Get and set component state
        <input type="text" value={this.email()} onchange={m.withAttr('value', this.email)}/>
      </div>;
  }

  config(isInitialized) {
    if (!isInitialized) {
      // 5. Initialize the DOM
      this.$('input').css('border-color', 'red');
    } else {
      // 6. Update the DOM
      this.$('input').css('border-color', 'blue');
    }
  }

  onunload(e) {
    // 7. Hook onto component dismount, and potentially prevent it
    if (this.email() !== 'toby@flarum.org') {
      e.preventDefault();
    }
  }
}

Mithreact

class Dropdown extends Component {
  // 1. Initialize props as they come in
  getDefaultProps() {
    return {icon: 'mail'};
  }

  componentWillReceiveProps(nextProps) {
    nextProps.className += ' bar';
  }

  // 2. Set initial state
  getInitialState() {
    return {email: 'toby@flarum.org'};
  }

  render() {
    return (
      <div className={this.props.className}>
        <i className={'fa fa-'+this.props.icon}/>
        // 4. Get and set component state
        <input type="text" value={this.state.email} onchange={this.handleEmailChange.bind(this)}/>
      </div>
    );
  }

  handleEmailChange(e) {
    this.setState({email: e.target.value});
  }

  componentDidMount() {
    // 5. Initialize the DOM
    this.$('input').css('border-color', 'red');
  }

  shouldComponentUpdate(nextProps, nextState) {
    // 3. Determine whether the component should redraw by dirty-checking a value
    return nextState.email !== this.state.email;
  }

  componentDidUpdate() {
    // 6. Update the DOM
    this.$('input').css('border-color', 'blue');
  }

  componentWillUnmount() {
    // 7. Hook onto component dismount, and potentially prevent it
    if (this.email() !== 'toby@flarum.org') {
      return false;
    }
  }
}

Advantages of Mithreact:

It's more or less the same as the React API, so people will be familiar with it
By dividing things up into more methods with less arguments, things are cleaner, and extensions will have an easier time monkey-patching
Component state is encapsulated so we can potentially get some performance gains for free

Disadvantages:

Lots of refactoring to do for core + existing extensions

Thoughts?

tobyzerner commented 8 years ago

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.

tobyzerner commented 8 years ago

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.

tobyzerner commented 8 years ago

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 tobscure/mithril-es6-components.

franzliedke commented 8 years ago

Looking good. :+1:

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?

tobyzerner commented 8 years ago

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...

Correct. We're still able to use any other Mithril components though, no matter what API we choose to implement.

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?)

Sounds good.

Do you want to host this under your account or under Flarum's?

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 :)

franzliedke commented 8 years ago

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?

tobyzerner commented 8 years ago

Yep. So any breaking changes we can just update in our abstraction.

tobyzerner commented 8 years ago

Some other JS API todos while I think of them (will create more issues later):

[ ] Remove attrs API from Post class. Subclasses can override render and add class names there instead
[ ] Deprecate utils/classList, use classnames instead
[ ] Fork es6-micro-loader to allow for direct monkey-patching of modules (so utils + helpers can be patched)
[ ] Standardise some component method names, like data vs. submitData
[ ] Make monkey-patching utils fail silently instead of crashing if some object/method doesn't exist
[ ] Think about the way extensions will interact with each other, expose their own APIs, monkey-patch
[ ] Refactor DiscussionControls etc. into functions rather than objects, remove Controls suffix from sub-methods
[ ] Make a flarum/lib namespace so that forum and admin apps can extend components/models with the same name
[ ] Replace app.request internals with fetch API instead of m.request [maybe]
[ ] Import app instead of using it as a global
[ ] Set app.current in mapRoutes, instead of in the Page component constructor. Rename to app.page.
[ ] Remove unused mixin util
[ ] Extract Translator into an external package

tobyzerner commented 8 years ago

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!

sijad commented 8 years ago

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.

jiayihu commented 8 years ago

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. If you are planning to build a React-like API, why don't just completely use React and gradually move away from Mithril?

dav-is commented 8 years ago

Because react is slow. Mithril is extremely Fast and lightweight. That's why it was chosen. Inferno may be a good candidate.

tobyzerner commented 8 years ago

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.

jiayihu commented 8 years ago

@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.

franzliedke commented 8 years ago

I think that migrating to React isn't anywhere in scope for the upcoming major release. ;)

edittler commented 8 years ago

React is not totally open source. See its patents. More info in this article.

jiayihu commented 8 years ago

@ezeperez26 Maybe you should read more carefully the patent.

The license granted hereunder will terminate, automatically and without notice, if you (or any of your subsidiaries, corporate affiliates or agents) initiate directly or indirectly, or take a direct financial interest in, any Patent Assertion: (i) against Facebook or any of its subsidiaries or corporate affiliates, (ii) against any party if such Patent Assertion arises in whole or in part from any software, technology, product or service of Facebook or any of its subsidiaries or corporate affiliates, or (iii) against any party relating to the Software.

Notwithstanding the foregoing, if Facebook or any of its subsidiaries or corporate affiliates files a lawsuit alleging patent infringement against you in the first instance, and you respond by filing a patent infringement counterclaim in that lawsuit against that party that is unrelated to the Software, the license granted hereunder will not terminate under section (i) of this paragraph due to such counterclaim.

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: https://github.com/facebook/react/issues/7293

franzliedke commented 7 years ago

For reference: This ticket can hopefully also be closed once we upgrade to Mithril 1.0.

dead-claudia commented 6 years ago

(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?

tobyzerner commented 6 years ago

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 {subtree: 'retain'} in Mithril 1.x?

dead-claudia commented 6 years ago

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 notifications@github.com 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 https://github.com/flarum/core/issues/872#issuecomment-364246495, or mute the thread https://github.com/notifications/unsubscribe-auth/AERrBBIAaM-bjldtlM98aci-hcefyptAks5tS2A6gaJpZM4HzsjR .

gctommy commented 6 years ago

React is not totally open source. See its patents

@edittler – not the case anymore now that it's under MIT license.

you are loosing some contributors because of this choice

@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).

renato commented 5 years ago

Now that beta8 is out, what are the current thoughts on this/any rewrite?

dead-claudia commented 5 years ago

@tobscure You have any word on how things are going for you? We're on the road to releasing v2, which is why I ask. (It's basically v1 with most of the bugs fixed, just several of them required breaking changes.)

franzliedke commented 5 years ago

@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.

dsevillamartin commented 4 years ago

@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. Apologies if I use terms such as vnode and state incorrectly - the terminology confuses me quite a bit.

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.

`m.mount` used to return the component instance

This was easily solved by adding a setter in the component constructor

vnodes do not modify the original component

In Flarum we keep references to other components like in our DiscussionPage https://github.com/flarum/core/blob/46e2e17c3c0853bd9b40437007f1cedc7bb8d285/js/src/forum/components/DiscussionPage.js#L197-L199 https://github.com/flarum/core/blob/46e2e17c3c0853bd9b40437007f1cedc7bb8d285/js/src/forum/components/DiscussionPage.js#L107-L109

We rely on having the latest instance of that element - in other words, the current Mithril state. With Mithril v2.x, as it clones the component interface and doesn't modify the original, I am unsure of how to proceed. We need to be able to execute methods in PostStream that affect the currently rendered stream, but what we have saved is an instance that isn't connected to anything - the vnode's state is different than what we can modify. For example, this.stream.element https://github.com/flarum/core/blob/d3ec99cb37654571c6cb9fc3910663c02d54aa35/js/src/common/Component.ts#L28-L31 is null when it is defined in the vnode's state.

I tried returning a component that simply runs the current instance's views & lifecycle hooks

screenshot

but this causes the DOM to be removed & recreated on every Mithril redraw as the check for if the object is equal is always false ({} !== {})

dead-claudia commented 4 years ago

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 vnode.state = ....

Regarding oncreate and onupdate, vnode.dom should always be set to the latest reference - could you reduce that to a minimal repro and file an issue with the version you used? I'd like to investigate further.

Also, your inline component will always fail anyways, because you're creating a new component each time, and Mithril diffs that via a simple vnode.tag === old.tag check. Have you considered returning a fragment instead? (Lifecycle methods can also be passed as attributes, and you can set those on fragments via m.fragment(attrs, ...children).)

dsevillamartin commented 4 years ago

@isiahmeadows Thank you for your response, I'll try what you suggested.

As for the vnode.dom thing, not sure if I explained myself correctly and/or am reading your response correctly. The issue is not that vnode.dom would be undefined, is that we are unable to access it from outside the component. Perhaps I need to try and explain this again, my apologies for that.

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 this.stream (for example) does not modify the rendered DOM because it is not the actual rendered instance, as it is cloned before rendering.

EDIT: When I said that this.stream.element is null, this.stream is the instance created by new PostStream() and NOT the output of vnode.state. Perhaps I didn't explain that correctly.

Also, your inline component will always fail anyways, because you're creating a new component each time, and Mithril diffs that via a simple vnode.tag === old.tag check.

Yeah, that's why it didn't work as a solution. Not sure how m.fragment would help here as I still need to pass the children...? Though I seem to have kinda figured something out with it.

dead-claudia commented 4 years ago

@datitisev You can pass lifecycle hooks in m.fragment and elements both, via m.fragment({oncreate(vnode) { ... }, ...children), m("div", {oncreate(vnode) { ... }, ...children), and similar, which helps in a lot of more advanced use cases where you'd otherwise need to instate an intermediate component. You can still pass children as usual, just you can also pass attributes (including keys).

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 malloc/free, just with some indirection to the referenced memory, so it's not as simple as a pointer dereference to get.)

dead-claudia commented 4 years ago

We rely on having the latest instance of that element - in other words, the current Mithril state. With Mithril v2.x, as it clones the component interface and doesn't modify the original, I am unsure of how to proceed.

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 m(SomeComp, ...)/<SomeComp ... />, you should be fine, and you can get rid of that render method entirely. This method is fine, since the this is the component class itself (and the component class will be invoked with new), but this method is not, since this is the component instance, and the this in each of the methods will be that subclass, not the initial instance.

For your PostStream specifically, if you want a surgical fix, try setting your scroll listener directly in oninit rather than in the constructor. That way, it gets created correctly without issue.

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 <ReplyPlaceholder{...props} /> rather than {ReplyPlaceholder.component({...props})}, as there's far fewer moving parts involved.

dsevillamartin commented 4 years ago

@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.

dead-claudia commented 4 years ago

Welcome!

luceos commented 4 years ago

@isiahmeadows Sorry to bother, I'm not the strongest technically speaking and I'm having a hard time understanding the meaning of this:

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 malloc/free, just with some indirection to the referenced memory, so it's not as simple as a pointer dereference to get.)

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 ❤️

dead-claudia commented 4 years ago

Where you would ordinarily pass the element (vnode.dom), pass the component instance (this === vnode.state) instead.

dead-claudia commented 4 years ago

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.

franzliedke commented 4 years ago

Haha, that's not going to apply for us, thankfully.

@isiahmeadows Thanks for the awesome support and for Mithril! :heart:

dsevillamartin commented 4 years ago

@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 @ https://github.com/flarum/core/commit/f39d0ab16101ae644fbd850e9d0a983e99ae29c4#diff-ccd768950e2518aaa4b440287d036a07.

And again, thank you for all your help so far.

dead-claudia commented 4 years ago

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.

dsevillamartin commented 4 years ago

@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?

dead-claudia commented 4 years ago

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 notifications@github.com 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 https://github.com/flarum/core/issues/872?email_source=notifications&email_token=ABCGWBHCFMWTLOH6ZIGLUN3RGYFZBA5CNFSM4B6OZDI2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOKYKXA#issuecomment-597001564, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABCGWBD6HSFGOLVB7RINRULRGYFZBANCNFSM4B6OZDIQ .

--

Isiah Meadows contact@isiahmeadows.com www.isiahmeadows.com

dsevillamartin commented 4 years ago

Ah, I see. Thank you for all your time and help.

askvortsov1 commented 4 years ago

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:

getDefaultProps() {
    return {icon: 'mail'};
  }

  componentWillReceiveProps(nextProps) {
    nextProps.className += ' bar';
  }

  // 2. Set initial state
  getInitialState() {
    return {email: 'toby@flarum.org'};
  }

dsevillamartin commented 4 years ago

@askvortsov1 Not sure.

askvortsov1 commented 4 years ago

The still relevent bits of this have been extracted out; the rest is no longer relevant as there's no clear direction here.

flarum / framework