Skip to content
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

Add and optional custom argument to mount/route/render for injecting dependencies into components #2355

Closed
brentropy opened this issue Jan 6, 2019 · 8 comments

Comments

@brentropy
Copy link

brentropy commented Jan 6, 2019

Description

Add an optional additional argument to mount(), route() and render() that will be passed through to a second argument when creating closure and class components. This will enable writing more testable, loosely coupled components.

Why

I really like how simple Mithril is. My only frustration with the framework is that I feel like it promotes writing tightly coupled code in some cases. For example, lets look at the first part of the tutorial app (all in one file for simplicity):

"use strict"

var m = require("mithril")

var User = {
  list: [],
  loadList: function() {
    return m.request({
      method: "GET",
      url: "https://rem-rest-api.herokuapp.com/api/users",
      withCredentials: true
    })
    .then(function(result) {
      User.list = result.data
    })
  }
}

var UserList = {
  oninit: User.loadList,
  view: function() {
    return m(".user-list", User.list.map(function(user) {
      return m(".user-list-item", user.firstName + " " + user.lastName)
    }))
  }
}

m.mount(document.body, UserList)

User is tightly coupled to m.request and UserList is tightly coupled to User. I know this is simplified for learning Mithril and there are things that can be done to improved this. For example we could use the factory pattern for the User model.

function createUserModel(request) {
  var User = {
    list: [],
    loadList: function() {
      return request({
        method: "GET",
        url: "https://rem-rest-api.herokuapp.com/api/users",
        withCredentials: true
      })
      .then(function(result) {
        User.list = result.data
      })
    }
  }
  return User
}

var User = createUserModel(m.request)

This works outside of rendering because Mithril lets you do whatever you want. However, if we left the rest of the example as is UserList is still tightly coupled to our initialized User instance, and indirectly m.request.

One solution would be to pass the User model to UserList as an attr. This would be really easy for this example, but as soon as multiple components rely on the User model it gets messy, requiring any component that renders that component to have the User model and pass it.

It would be nice to have a single place to initialize all of the would be singleton models/services and make those instances available to all components without explicit passing.

Some frameworks opt for more complicated dependency injection containers, but in the spirit of Mithril simplicity this could be accomplished with this extra argument. The end result would be this:

"use strict"

var m = require("mithril")

function createUserModel(request) {
  var User = {
    list: [],
    loadList: function() {
      return request({
        method: "GET",
        url: "https://rem-rest-api.herokuapp.com/api/users",
        withCredentials: true
      })
      .then(function(result) {
        User.list = result.data
      })
    }
  }
  return User
}

function UserList(vnode, context) {
  var User = context.User
  return {
    oninit: User.loadList,
    view: function() {
      return m(".user-list", User.list.map(function(user) {
        return m(".user-list-item", user.firstName + " " + user.lastName)
      }))
    }
  }
}

m.mount(document.body, UserList, {
  User: createUserModel(m.request)
})

This is a small change with little cost that could have a big impact in helping people write more testable Mithril components.

Possible Implementation & Open Questions

Recommended approach

  • value optionally passed to render as third argument
  • threaded through as last argument for:
    • initComponent
    • createComponent
    • updateComponent
    • createNode
    • updateNode
    • createNodes
    • updateNodes
    • createFragment
    • updateFragment
    • createElement
    • updateElement
  • passed to closure/class components from initComponent

Benefits

  • non-breaking change
  • no noticeable memory usage or rendering performance impact (tested with db monster)
  • only adds 48 bytes to the final gziped file

Drawbacks

  • adds some argument passing mess to render.js
  • ?

Is there a better way to approach this with Mithril? Are there issues that I am overlooking?

What should this argument be called? I called it context in my code. I think that makes sense since the value will be context (top level render) specific. However, there may be confusion for people that expect context to mean the same thing as it does in react. This could be used for many of the use cases that
react context gets used for, but it is very different (i.e. components don't set a context for their children). Also, I see there is an open request to implement a react-like context in mithril (#2148).

Is this something you're interested in working on?

Yes!

@dead-claudia
Copy link
Member

Strong 👎 from me because you can just use attributes for injecting arguments into top-level components:

var User = createUserModel(m.request)
m.mount(document.body, {
	view: function () { return m(UserList, {User: User}) }
})

In this case, you would access the model via vnode.attrs rather than context, like so:

var UserList = {
	oninit: function (vnode) {
		var User = vnode.attrs.User
		User.loadList()
	},
	view: function (vnode) {
		var User = vnode.attrs.User
		return m(".user-list", User.list.map(function (user) {
			return m(".user-list-item", user.firstName, " ", user.lastName)
		}))
	},
}

Otherwise, it works almost exactly the same as how your proposal would.

@brentropy
Copy link
Author

@isiahmeadows, I don't think I did a great job of communicating what I am really after. If it was only an issue of top-level components then the attribute solution would be great. I tried to call that out with:

One solution would be to pass the User model to UserList as an attr. This would be really easy for this example, but as soon as multiple components rely on the User model it gets messy, requiring any component that renders that component to have the User model and pass it.

I guess my example illustrates how it would work, and not necessarily when it is really needed. I would like to be able to inject dependencies in to any component not just top-levels components. When I have a little more time today I will put together a few better examples.

@brentropy
Copy link
Author

brentropy commented Jan 7, 2019

@isiahmeadows an example that may better illustrate this even though I left most of the details out:

"use strict"

var m = require("mithril")

function createUserModel(request) {
  return {
    // ...
  }
}

function createChatService(socket, redraw) {
  return {
    // ...
  }
}

var Page1 = {
  view: function() {
    return m("main", [
      "Page 1",
      m(ChatBox),
      m(TranslatedButton)
    ])
  }
}

var Page2 = {
  view: function() {
    return m("main", [
      "Page 2",
      m(ChatBox)
    ])
  }
}

function ChatBox(vnode, context) {
  var User = context.User
  var ChatService = context.ChatService
  return {
    // component that depends on User and ChatService
  }
}

function TranslatedButton(vnode, context) {
  var t = context.translations
  return {
    view: function() {
      return m("button", t.submit)
    }
  }
}

m.route(document.body, "/page1", {
  "/page1": Page1,
  "/page2": Page2
}, {
  // things that can be injected in to any component
  User: createUserModel(m.request),
  ChatService: createChatService(
    new WebSocket("ws://chathost"),
    m.redraw
  ),
  translations: {
    submit: "Entregar"
  }
})

In the case of the ChatBox component it depends on the User model and and instance of ChatService. It is rendered by multiple top-level components and those are things may be less ideal to explicitly pass along the way via attributes.

Yes we could pass User and ChatService explicity as attributes to Page1 and Page2, and each of those could in turn pass both as attributes to ChatBox, but that can get really messy, especially in a really large app.

The TranslatedButton component demonstrates how this could be used to inject common config that is available to any component rendered in that context. It wouldn't make a lot of sense to require a parent component rending TranslatedButton to have the translation and pass it as a attribute. With the context it has access to that configuration without explicit passing or directly relying on globals.

@barneycarroll
Copy link
Member

This is a divisive subject. There's an interesting juxtaposition in contemporary web development wisdom in that, while with CSS-in-JS we've come to the conclusion that the cascade aspect of cascading-style-sheets is too blunt a tool to credibly account for the necessary complexity of styling in web applications, the hierarchical cascade has been reimplemented as a tool for application modelling dependency injection in the form of context.

I'm of the inclination that the context pattern shouldn't be a first-class implementation concern for Mithril. In my experience from working with React it is generally used as a blunt instrument to cater for 2 broad concerns: dependency injection and application model data-binding.

In the latter case (veering off topic a bit, but, for the sake of completeness), there is an inevitable dependency on the Javascript runtime (model data is dynamic) and there is often a virtual DOM hierarchy coincidence (multiple co-existent instances of model item X, with a 1-to-1 correlation to virtual DOM structure Y). But the solution is to pass these items by reference.

For the purposes of dependency injection - and this also applies to React's canonical example of theming - there is never any salient relationship to virtual DOM hierarchy, or even application runtime concerns. Rather than hinging on a user interface for implementing these concerns I would suggest implementing these as part of a build script, or bundler config (package aliasing). In the cases where there really are runtime concerns, or for the sake of developer ergonomics, there is always the trusty old mechanism of global namespacing.

So, in brief:

  1. Is it globally unique per application?
    1. No: pass by reference
    2. Yes:
      1. Is it required to be defined or changed over the course of application runtime?
        1. No: module aliasing / build scripting
        2. Yes: global namespace assignment

@barneycarroll
Copy link
Member

Worth going over this here as a tying together of the analysis over on #2148, namely seeing context in React as a design accident.

As mentioned a couple of times in that thread, React needed something functionally like context because of the way it implemented redraw, namely as an opinionated process that was a granularly-handled function of an internal state differentiation: in Mithril this is the other way round, with granular differentiation being a function of a top-down draw instruction (that happens to be triggered by Mithril-bound user / server IO by default). There could be a host of intersecting reasons for why they decided to do this: my opinion is that for one, React was never as function-oriented in its design as it came to be seen in popular opinion: the focus on classes and JSX as the primary idioms belie (by intention or accident) a mindset still deeply rooted in the comfort blanket of OOP tropes and stringy templates. By contrast Mithril is much more embracing of raw Javascript (hyperscript), functions and functional graphs.

It's worth noting that React officially discouraged public access to the context API for application logic modelling up until very recently: the climb down can be interpreted as a concession to the overwhelming popularity of Redux (which itself was a fan-made project to fix embarrassing shortcomings in React's own Flux implementation).

Related to this is the fact that React are now moving to discourage using classes at all, which lends weight to the notion of early API design mistakes which they are forced to maintain because of overwhelming popularity. Mithril isn't burdened by this and we owe it to ourselves to stand on the shoulders of giants rather than tread on the toes of elves. Previously we have made questionable API forking options pandering to perceived popularity - class components (appeal to React popularity), route resolvers (appeal to Webpack popularity). These are similarly uncritical offerings that are difficult to rationalise from first principles and even harder to justify in hindsight: users occasionally end up using these charitable offerings provided by authors who don't use them themselves and end up facing problems we find difficult to solve because we never really believed in the underlying mechanisms anyway. IMO this is irresponsible of us, especially when Mithril has nothing to gain from stealing a market predisposed to React for reasons of comfort.

As of late we've edged towards being a bit more confident in our opinions and responsible in our API provisions, discouraging the use of our vdom's internal state mechanisms as application modelling tools and preferring functional constructs on top of vdom instead of within.

@brentropy
Copy link
Author

@barneycarroll I completely agree with regard to React's context. I am not a fan of how it is implemented. While what I am proposing could be used for much of what react context ends up being used for, I was actually more inspired by withExtraArgument in redux-thunk than react's context. It is incredibly simple, but I have found withExtraArgument to be extremely useful and has helped promote better design and testing when used.

The only thing this has to do with the virtual DOM is that it owns creating component instances. I would like to be able to give it a value to blindly pass along since it is one of the few things I don't have direct control over with Mithril.

I understand (and appreciate) the caution toward committing to API additions and the impact that can have on maintaining the project going forward. Based on current feedback it seems unlikely, but I do think this simple change could offer a lot of value.

Here is the working change for reference. The only changes are insertions of , context.
brentropy@734592e

@dead-claudia
Copy link
Member

@brentburg This is basically asking for context, just a more restricted form that can only be set at the top level as opposed to anywhere in the tree.

Here's your example, recast to work with today's APIs:

In ES5, it's a tad verbose
"use strict"

var m = require("mithril")

function createUserModel(request) {
    return {
        // ...
    }
}

function createChatService(socket, redraw) {
    return {
        // ...
    }
}

var Page1 = {
    view: function(vnode) {
        var context = vnode.attrs.context
        return m("main", [
            "Page 1",
            m(ChatBox, {context: context}),
            m(TranslatedButton, {context: context})
        ])
    }
}

var Page2 = {
    view: function() {
        var context = vnode.attrs.context
        return m("main", [
            "Page 2",
            m(ChatBox, {context: context})
        ])
    }
}

function ChatBox(vnode) {
    var context = vnode.attrs.context
    var User = context.User
    var ChatService = context.ChatService
    return {
        // component that depends on User and ChatService
    }
}

function TranslatedButton(vnode) {
    var context = vnode.attrs.context
    var t = context.translations
    return {
        view: function() {
            return m("button", t.submit)
        }
    }
}

var context = {
    // things that can be injected in to any component
    User: createUserModel(m.request),
    ChatService: createChatService(
        new WebSocket("ws://chathost"),
        m.redraw
    ),
    translations: {
        submit: "Entregar"
    }
}

m.route(document.body, "/page1", {
    "/page1": {view: function() { return m(Page1, {context: context}) }},
    "/page2": {view: function() { return m(Page2, {context: context}) }}
})
ES6+ has features that help cut down tremendously on the boilerplate.
"use strict"

const m = require("mithril")

function createUserModel(request) {
    return {
        // ...
    }
}

function createChatService(socket, redraw) {
    return {
        // ...
    }
}

var Page1 = {
    view({attrs: {context}}) {
        return m("main", [
            "Page 1",
            m(ChatBox, {context}),
            m(TranslatedButton, {context})
        ])
    }
}

var Page2 = {
    view({attrs: {context}}) {
        var context = vnode.attrs.context
        return m("main", [
            "Page 2",
            m(ChatBox, {context})
        ])
    }
}

function ChatBox({attrs: {context}}) {
    const {User, ChatService} = context
    return {
        // component that depends on User and ChatService
    }
}

function TranslatedButton({attrs: {context}}) {
    const t = context.translations
    return {
        view: () => m("button", t.submit)
    }
}

const context = {
    // things that can be injected in to any component
    User: createUserModel(m.request),
    ChatService: createChatService(
        new WebSocket("ws://chathost"),
        m.redraw
    ),
    translations: {
        submit: "Entregar"
    }
}

m.route(document.body, "/page1", {
    "/page1": {view: () => m(Page1, {context})},
    "/page2": {view: () => m(Page2, {context})}
})

Given that this is in fact possible in userland and it works mostly the same way it would with native context, I'm closing this as a duplicate of #2148.

@dead-claudia
Copy link
Member

Duplicate of #2148

@dead-claudia dead-claudia marked this as a duplicate of #2148 Jan 17, 2019
@dead-claudia dead-claudia moved this to Completed/Declined in Feature requests/Suggestions Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Completed/Declined
Development

No branches or pull requests

3 participants