Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Mithril Realworld Example App #1823

Closed
barryels opened this issue Apr 28, 2017 · 23 comments
Closed

Mithril Realworld Example App #1823

barryels opened this issue Apr 28, 2017 · 23 comments
Labels
Type: Meta/Feedback For high-level discussion around the project and/or community itself

Comments

@barryels
Copy link
Contributor

barryels commented Apr 28, 2017

I'm busying setting up a Mithril implementation of the Realworld app.

Would really appreciate feedback / contributions at barryels/realworld-mithril or (more high level) at: gothinkster/realworld#69

@tivac
Copy link
Contributor

tivac commented Apr 28, 2017

This is a neat idea, will take a look this evening and probably send a few PRs.

@barryels
Copy link
Contributor Author

Brilliant, thanks @tivac 👍

@barryels
Copy link
Contributor Author

@barryels
Copy link
Contributor Author

barryels commented Apr 29, 2017

Hi everyone, just an update:


TL;DR

  • Question 1: Should example Mithril code be written in es6 + jsx, es6 + m(), es5 + jsx or es5 + m()?
  • Question 2: Is showcasing the "layoutability" of m.route() overkill / distracting?
  • Question 3: Should I simplify the domain.js + api-adapter.js setup?
  • Question 4: Tests?

  • The app's about 50% complete!
    I've been focusing on a broad strokes implementation of the "Realworld" spec and trying not to concern myself with too many details. So there are some gaps, e.g. multiple filtered ArticleLists (probably best to utilise a higher order component to wrap some related concerns), etc.

  • Question 1: Should example Mithril code be written in es6 + jsx, es6 + m(), es5 + jsx or es5 + m()?
    The other "Realworld" impls tend toward es6 syntax + webpack / rollup. Whereas I tried to keep the Mithril version simple and "old-school" (es5 + m() + browserify). Since these impls will probably be scrutinised by senior devs (as a framework comparison tool), I don't want the Mithril impl to be relatively undermined. Any thoughts?

  • Question 2: Is showcasing the "layoutability" of m.route() overkill / distracting?
    The other impls tend toward direct routing to a "Page" component without wrapping pages in a layout, so it's probably best to do the same; for the sake of apples-to-apples comparison. On the other hand; layout comp wrapping showcases how simple and idiomatic Mithril code is.

  • Question 3: Should I simplify the domain.js + api-adapter.js setup?
    This is a tough one for me. I (as most) like having the business rules portion of an app completely divorced from the UI, but doing so without creating an abstraction layer that is distracting / confusing can be a bit difficult. Since Mithril handles XHR (unlike some of the other impls) I don't want to obscure it too much behind an implementation that, to a passerby, may seem complex and then they judge Mithrils library-specific design decisions on something that is app-specific.

  • Question 4: Tests?
    Doing TDD is like flossing; everyone knows it's a good thing to do, but it's just so bloody hard to be consistent ;)

I'm extremely excited about Mithril (have been using it in production for at least a year) and would love for it to become more mainstream, it's one of those libs that allows you to leave work at 17:00 👍 Thanks to everyone for helping to grow and maintain this project!

@tivac
Copy link
Contributor

tivac commented Apr 29, 2017

  1. It's probably the most "idiomatic mithril" to do ES5 + m(), but use whatever you're comfortable with IMO. We use ES6 + m() and transpile.
  2. I think RouteResolvers + Layout components are rad, so I vote for using 'em
  3. KISS, with comments about potential improvements & further abstractions if necessary. We tend to use a single state object w/ a simple functional API.
  4. ¯\_(ツ)_/¯ if other "RealWorld" projects have 'em it's probably a good idea.

@orbitbot
Copy link
Member

  1. One of the first thoughts that I had when learning about this project is that it would leave room for different interpretations, not only in how to organise the views / controllers and such, but also in how to implement the appropriate model. I've avoided looking at some implementations in the interest of maybe giving it a go on my own at some later point. I would 👎 on using JSX if you feel like writing an "idiomatic" app. On the other hand, it might be easier for React people to compare with JSX. Over time it would probably be good to have multiple projects with different techniques, so the important thing would perhaps be to label/rename the repo properly, since that seems to be how RealWorld sorts different implementations.

  2. I'd use Mithril to its full potential as it makes sense to you.

  3. Haven't looked at the implementations, don't understand the question 😉

  4. I'm for BTDD on principle, however don't really do that much in throwaway projects, and the money is in testing the model layer rather than the view layer anyways...

@barryels
Copy link
Contributor Author

Awesome! Thanks for the feedback @tivac

  1. Will leave it as-is then (ES5 + m()).
  2. Cool, will leave as-is for now, possibly adding a second layout comp to wrap a screen or two at a later stage.
  3. Rad, I'll conjoin and simplify domain & api-adapter.
  4. The other projects don't seem to, but it could be a good opp to showcase mithrils simple testing approach.

@StephanHoyer
Copy link
Member

StephanHoyer commented Apr 29, 2017

What about SSR? Do you plan to do this? I can assist in this space if you want

@barryels
Copy link
Contributor Author

Thanks for the feedback @orbitbot :)

  1. Yeah, was initially going to create a mithril-mobx version, but wanted to try an "idiomatic" approach first. Looking at the other projects has caused me to change my approach in some areas, so as to ensure that the Mithril version is as close to an "apples-to-apples" comparison as is possible. The main reason for alignment with the other projects is to help senior devs easily grok the libraries' differences without getting distracted by app-level details. We can rename the repo later on when the need arises :)
  2. hehehe
  3. Yeah, my thinking as well... very little ROI in "visual" UI testing. Will probably set up a suite for the domain object and ignore the UI for now, unless someone else wants to pick that up ;)

@barryels
Copy link
Contributor Author

@StephanHoyer Oh cool! Hadn't planned on it, but if you're offering... Then, hell yes!

@barryels
Copy link
Contributor Author

barryels commented Apr 29, 2017

@tivac Re: 3. domain + api-adapter

Do you mind posting a simple example of what your implementation looks like?
I've just retired api-adapter... domain now handles all app-level concerns, might rename it to app.js ;)

@dmitriz
Copy link
Contributor

dmitriz commented May 3, 2017

Just my few cents for what it is worth.
I have some Angular experience but got curious with Mithril for it's no nonsense approach...
So apologies for my ignorance in advance if there is something I miss.

So following that principle, this

var routes = {
	'/': {
		view: function () {
			return m(LayoutDefault, m(ScreenHome));
		}
	},
	'/article/:slug': {
		view: function () {
			return m(LayoutDefault, m(ScreenArticle));
		}
	},
	'/register': {
		view: function () {
			return m(LayoutDefault, m(ScreenUserRegister));
		}
...
}

might be perhaps possible to shorten to

var routes = {
	'/': () => m(LayoutDefault, m(ScreenHome)),
	'/article/:slug': () => m(LayoutDefault, m(ScreenArticle)),
	'/register': () => m(LayoutDefault, m(ScreenUserRegister)),
...
}

or even

var routes = {
	'/': LayoutDefault(ScreenHome),
	'/article/:slug': LayoutDefault(ScreenArticle),
	'/register': LayoutDefault(ScreenUserRegister),
...
}

The last one would be a portable general syntax with no specific library dependency,
and that universality may appeal to people coming from other frameworks (it would to me).

You can even go "functionally crazy" and write it like

var routeConfig = {
	'/': ScreenHome,
	'/article/:slug': ScreenArticle,
	'/register': ScreenUserRegister,
...
}

var routes = R.map(LayoutDefault, routeConfig)
// or
var routes = routeConfig.map(LayoutDefault)

From the code organisation perspective, the components directory looks very busy. Any reasons all components, even unrelated ones, must sit next to each other? This "by-feature-vs-by-function" organisation discussion had been quite popular in angular circles for a quite a while, with the outcome in definite favour of the features.

I have found the directory structure really well done in the angular real world example, with no directory having more than 10 or so files.

Perhaps I would move the LayoutDefault to the higher level, due to its hierarchical role.


Another things that might make it easier to play and rearrange folders,
could be to use absolute paths instead of

var domain = require('./../../domain');

Nothing major, but could make life sweeter a bit :)

Or am I missing any downside here?

@tivac
Copy link
Contributor

tivac commented May 3, 2017

@dmitriz That's not how RouteResolvers work. They're a specifically-shaped object that mithril treats differently to enable persistent layout components.

@dmitriz
Copy link
Contributor

dmitriz commented May 3, 2017

@tivac
Sure.

I am just saying it might be simpler to write the routes that way for the library consumer, so might be worth to support pure functions instead of the more verbose and complex components, where pure view functions is quite often (always in this app) all you need.
(The idea is from @JAForbes)

Basically, there are two ways:

  • Extend the router functionality to recognise functions in place of components and simply act as if those are the view methods;
  • Wrap the router in a new object that would do just that.

The first way would make it more user-friendly in my view.

Does it make sense?

@dmitriz
Copy link
Contributor

dmitriz commented May 3, 2017

@barryels
Copy link
Contributor Author

barryels commented May 3, 2017

@dmitriz Thanks for the feedback, quite valuable discussion points.

  1. Router refactor (RouteResolver):
    I think one of the goals of this project should be to keep things as simple as possible. Whilst I agree with your sentiment completely (this project source is not a 1:1 version of what I'd build for clients). Each person / team will have their own approach to abstractions, and it would probably hinder the understanding of mithril to add too many layers.
    Your example refactoring exactly illustrate the power that comes from mithril's simplicity :)

  2. File structure
    Yup, I'm definitely in the structure-by-feature camp (have worked on some massive angular apps, with feature-toggling, etc.), and organising by feature works most of the time, but there will eventually be a feature request which cross-cuts multiple existing features. Once again, this is a subjective decision, which I'm not willing to make if it's going to detract from mithril's implementation details.
    Probably best to move Screen*.js and Layout*.js into separate dirs, but other than that I'm not sure that the advantage of feature grouping will outweigh the disadvantage of "hiding" components in separate directories... at this point, at least :)

Example

This component:
https://github.com/gothinkster/angularjs-realworld-example-app/master/src/js/components/buttons/favorite-btn.component.js

should probably live here:
https://github.com/gothinkster/angularjs-realworld-example-app/master/src/js/article

  1. Absolute require's
    Nah, absolute pathing in cjs is not a good idea, it creates more problems than it solves; most notably lack of consistent cross-IDE support :/

@barryels
Copy link
Contributor Author

barryels commented May 3, 2017

@dmitriz

Is there a reason for the null value for what will be called as function?

Was just defining the shape of the state object, so that it's clear that some function is going to have to update that reference :)

@orbitbot
Copy link
Member

orbitbot commented May 3, 2017

Re: absolute paths, methinky something like rollup's root import is what you'd want, if you're aware of it or not ;)

@barryels
Copy link
Contributor Author

barryels commented May 3, 2017

Ah, yes, requires using es6 modules though?

@orbitbot
Copy link
Member

orbitbot commented May 3, 2017

Perhaps in this specific case, there are probably alternatives for other environments & toolchains...

@dmitriz
Copy link
Contributor

dmitriz commented May 3, 2017

@barryels

Thanks for the feedback, quite valuable discussion points.

You are welcome!

Router refactor (RouteResolver):
I think one of the goals of this project should be to keep things as simple as possible. Whilst I agree with your sentiment completely (this project source is not a 1:1 version of what I'd build for clients). Each person / team will have their own approach to abstractions, and it would probably hinder the understanding of mithril to add too many layers.
Your example refactoring exactly illustrate the power that comes from mithril's simplicity :)

I can see your point. I still hope it can be made less verbose when staying simple.
As a workaround, perhaps make the router declarations more direct by importing the components rather than their view methods?

Also, does the router API not allow for any tree hierarchy, where I can put the defaultLayout as a top route and so avoid repeating it every single time?

File structure
Yup, I'm definitely in the structure-by-feature camp (have worked on some massive angular apps, with feature-toggling, etc.), and organising by feature works most of the time, but there will eventually be a feature request which cross-cuts multiple existing features. Once again, this is a subjective decision, which I'm not willing to make if it's going to detract from mithril's implementation details.
Probably best to move Screen*.js and Layout*.js into separate dirs, but other than that I'm not sure that the advantage of feature grouping will outweigh the disadvantage of "hiding" components in separate directories... at this point, at least :)

I am myself against deep nesting that are often abused with zillions of tiny files doing almost nothing. But your components are already 2 levels deep, so it wouldn't increase any depth with more subdirs inside src. Right now, as an outsider, I have no idea how to differentiate between different components. Perhaps putting those called by the router higher as index files in their same named directories with other dependent ones beside, then have a separate folder shared with the reusable ones that don't belong elsewhere. I just don't find the name components of much help when everything is a component :)

@barryels
Copy link
Contributor Author

barryels commented May 3, 2017

@dmitriz

RouteResolver: Something more like this? i.e. just DRYing the code up a bit?

var routes = {
	'/': setupRoute(ScreenHome),
	'/article/:slug': setupRoute(ScreenArticle),
	'/register': setupRoute(ScreenUserRegister),
	'/login': setupRoute(ScreenUserLogin),
	'/@:username': setupRoute(ScreenUserProfile),
	'/@:username/favorites': setupRoute(ScreenUserFavorites),
	'/settings': setupRoute(ScreenUserSettings),
	'/editor': setupRoute(ScreenEditor),
	'/editor/:slug': setupRoute(ScreenEditor)
};


function setupRoute(screen, layout) {
	layout = layout || LayoutDefault;

	return {
		view: function () {
			return m(layout, m(screen));
		}
	};
}

@dmitriz
Copy link
Contributor

dmitriz commented May 3, 2017

That would look nicer to me!

Or even restrict the router to the intermediate main slot, then you won't need to wrap them at all. Would that make sense?

@dead-claudia dead-claudia added the Type: Meta/Feedback For high-level discussion around the project and/or community itself label Oct 28, 2018
@MithrilJS MithrilJS locked and limited conversation to collaborators Jan 29, 2022
@orbitbot orbitbot converted this issue into discussion #2727 Jan 29, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
Type: Meta/Feedback For high-level discussion around the project and/or community itself
Projects
None yet
Development

No branches or pull requests

6 participants