We need to make life easier for new contributors to get onboard

dead-claudia commented 7 years ago

/cc @lhorie @tivac

We're in need of a lot of easy, basic, trivial pull requests that don't require much knowledge of the framework to actually complete. Additionally, the documentation could use a little enhancement, and there's an open issue for writing out the TypeScript definitions for the rewrite's API, which mostly only requires reading the docs a little more thoroughly (it wouldn't be as beginner-friendly, but it's doable for someone only mildly familiar with the contribution process). Also, I'm quite frankly getting tired of constantly dealing with people not testing their patches, or even in some cases, writing tests for them. (I'm not going to link those here, but I bet you can find several just looking through the existing PRs.)

I think we should create a new tag for beginner-friendly issues called easy, just for this very purpose, and be willing to work with new contributors on learning the process. This will also require us to do the following:

Clean up the rewrite's CONTRIBUTING.md file - in its current state, it's junk. It's missing a table of contents, a step-by-step guide of how to contribute, among other things. A single long FAQ isn't going to cut it if we want to actually guide users on how to contribute fixes to the framework. Although we currently have a usable one, the rewrite has enough differences in the workflow and style, it might be a good idea to start from scratch.
- The style guide should be migrated into a different file, so the contributing guidelines remain focused on just the general contribution process.
- The FAQ also needs reformatted (smaller headers), and a third of it would be redundant after being filled in with the rest of the details.
- I myself may end up creating the PR for all this, if I can find the time (which there is hope now :smile:).
Clean up the ospec docs a bit, to help with the previous point. It would be much easier to follow if the headers weren't C-like function prototypes complete with types, and the API docs were in a separate file. New contributors will have to know that API if they plan on writing any tests, and clean documentation will make their life much easier. Plus, people are already using it outside of Mithril space, so that will help.
Clean up the style a bit, like applying a line length limit so people don't have to scroll right just to read the code (I don't care what the limit is as long as it's no more than 120 characters), apply some more vertical whitespace and add some explanatory comments, so people don't have to decipher a 100% comment-free blob of code, and splitting up large, complex functions that don't really explain what each branch does.
Tag some of the existing bugs (like those I linked above) with easy and document the tag, so new contributors know where to look. Also, recolor the existing tags, so they're a bit more obvious at a glance for those of us more familiar.
Add some sort of roadmap to the wiki, linked directly from the README and eventual website, that we can refer people to so we can answer those questions much faster and get more efficient help with. TypeScript has a nice one that I feel we could use as a good starting point.

dead-claudia commented 7 years ago

@lhorie PS: I think we should change the main branch to rewrite now (or soon), since it's close to release time.

lhorie commented 7 years ago

Most of these are already in my TODO list, just lower priority than getting the tuts done :)

The only thing I disagree w/ is the line limit. Screens are really big these days and breaking lines mid-argument-list like in C++ is not super idiomatic (and personally, I find it difficult to read)

cmawhorter commented 7 years ago

like applying a line length limit so people don't have to scroll right just to read the code

I'd echo this as a general gripe about browsing github on mobile. with long lines it's very difficult to follow when you're learning something new.

getting the tuts done

This is the best way for beginners to learn. Break them out into issues and I'd be willing to tackle one or two as a learning exercise.

barneycarroll commented 7 years ago

Managing open source contributions is always tough, but from where I'm standing it's not as if Mithril's in any kind of crisis - v0.2 & v1 look in great shape! I appreciate that as a contributor concerned with the task of administrating open issues and PRs, this may look different :)

Also, I'm quite frankly getting tired of constantly dealing with people not testing their patches, or even in some cases, writing tests for them. (I'm not going to link those here, but I bet you can find several just looking through the existing PRs.)

I suspect you're saying you don't want to link to those because you feel that'd be like shaming enthusiastic contributors and you wouldn't want to turn them away or make them feel their work isn't welcome. But it could go someway to helping resolve them - some people like writing tests. What about a needs-tests label?

barneycarroll commented 7 years ago

Speaking as an occasional contributor, some of my concerns (which might make me contribute more):

Browser support

One of the easy fixes @isiahmeadows mentioned held an apparent concensus between himself and @pygy that v1 no longer cared for IE<9 support. What is v1's support matrix? Can we get it written into the README?

Rewrite isn't a branch, it's a separate project living in the same repo

I agree it's mildly annoying that next is the default branch. To reiterate what others have said on chat, this is because in order to make pull requests via Github's web interface, you need to load a page which attempts to diff the patch against the default branch, which is a costly and futile exercise.

This is a symptom of a wider problem: diffing next against rewrite is a nonsense. Moreover, rewrite is a terrible name for a branch because it implies it will at some point be able to be merged into master - in fact the opposite is true: it was never branched off next - it was rewritten from scratch. It's a totally different codebase. I tried to impress back before the rewrite was made public: why is rewrite a branch and not its own repo?

You can duplicate repos of course and try and address this frustration holistically, but that comes with a heavy heavy price: github doesn't appear to have a mechanism for cloning or auto-fixing references to issues and other github metadata, so you end up with broken or misleading references. Check out my attempts to do this by creating a clone of this original repo. So you could conceivably clone to mithril.js-v0.X and chuck in a note to say all the issues are over at mithril.js, but essentially as soon as that clone starts growing issues then the reference errors happen (people stumbling on old commits will think these are referencing new issues). In other words, you'd only want to do this for a dead and locked project for posterity, by which point you don't care about development hassle anyway.

For people who try to actively contribute to both branches, this problem is far more pronounced, because attempting to checkout one branch from the other results in wiping the whole codebase and writing it over again, with the extra ambiguity of npm dependencies floating around in rewrite because they're not versioned. How do people get around this? What's the best workflow for dealing with old and new?

What next for v0.X? (about those tests)

I'm hoping that when v1 breaks out of rc, v0.X can be retired. Is that the plan? If not, is it worth talking about the tests? My text editor struggles with the test file. I don't want to go into the sordid history of test rewrites, but is there some kind of compromise we could reach to avoid maintenance being any more depressing a chore than it already is?

Style guide, drawing in new users, contribution guidelines

I think more documentation and process often turns people off contribution. I think right now a lot of contributors make a drive by issue or patch, Isiah complains at them, and then things hang, and he worries they might have taken offence (but they probably haven't, they're probably just too busy), but complaining up front is unlikely to make things any better IMO. Ember had huge trouble soliciting casual contribution because [redacted] and the hoops they asked you to jump through were too anal (didn't chime well with the cartoon hamster who lured you in to the project), How about instructions to run npm run propose, which would confirm author intent and offer them the option of branching before a) validating that git diff returns something b) testing c) linting d) building e) pushing?

That way you cut down on explanation up front and after, you automate as much as possible, and you make life easier for the core contributors who spend less time doing admin and human resources and more time discussing cool new stuff :)

lhorie commented 7 years ago

@barneycarroll realistically speaking, I don't really have the time to spend on large efforts in 0.2.x, especially considering it will get closer and closer to end-of-life as v1.0+ matures. So I'll be relying on you guys more for 0.2.x efforts. What I do wrt to switching branches is just clone the repo twice. Then I can have both next and rewrite side by side.

re: contribution guidelines, I agree with the general sentiment here: it needs to be less intimidating and more welcoming.

re: browser support: I think I had the baseline at IE8 (assuming you're aware of XHR limitations), but I forget. It definitely can't be lower than IE8 due to reliance on "a"[0] syntax, but I don't remember if there was anything specifically preventing IE8 (there may have been some that slipped in via PRs)

pygy commented 7 years ago

@lhorie the mocks and the CommonJS polyfill rely on getters/setters (IIRC since the first rewrite commit), and you previously stated that the new baseline was IE9.

AFAIK, Mithril itself should work on IE8 (maybe even lower with polyfills if string indexing was done with .charAt(x)), but since the tests won't run it's hard to know for sure.

While getters/setters can't be polyfilled, IIRC the subset that Mithril in the mocks uses could be shimmed using — dramatic pause —VBScript. The project has little to no docs IIRC, butit has tests.

Not sure for the CJS polyfill, but I think it could be made to rely on little-loader + some between-scripts shennanigans rather than <script> tags and getters/setters.

I've actually already explored the question, but didn't want to bother you since the official baseline was IE9.

barneycarroll commented 7 years ago

OK so as a point of order shall we say v1's baseline support is full ES5 support (with caveats for IE9 issues)? I feel this is reasonable. My only niggling concern (which may be ephemeral) is Mithril's Japanese community that I occasionally stumble upon, combined with hear say that Japan as a country has an exceptional culture of legacy locked down Microsoft systems (ie many people at work using corporate networks based on old Windows OS versions kept alive past their sell-by-date). That's all anecdotal FUD, BTW — no evidence to show for it.

At the risk of chucking another tangent in the thread, how comfortable are we with browser compatibility testing? While the mocking approach seems great and we rarely get issues filed for engine-specific edge cases, getting a more holistic working method on this could ease the burden of what I interpreted as @isiahmeadows' issue admin fatigue — for example the languishing case of whether we need to patch for Safari < 6 failure to implement insertAdjacentHTML on SVG nodes. Is that an issue anyone has strong opinions on, or can we grin/scowl and bear it?

Edit: @lhorie thanks for the separate folder solution. So obvious and simple in hindsight. Just did this and my complaints about branch switching are totally resolved. :)

bruce-one commented 7 years ago

Random thoughts from a member of the peanut gallery (ignore at will :-) ):

needs-tests tag sounds like a great idea
- Similarly, where plausible, I think adding as much context of what's required for a languishing PR/issue to get merged would be awesome (imo) (but may be too much work - I just mean a comment like To get merged this PR would need: X (where X = "more tests", "more compatibility testing" etc))
Having IE8 support (via polyfills) was always a really useful trait of Mithril for us (even if we didn't always build for IE8, the fact we could add support retrospectively was very important).
Very rigid contribution guidelines are often a turnoff for me (but I understand them), my personal preference would be to preface the whole CONTRIBUTING.md with "When in doubt, make the PR, an incomplete PR is better than nothing!" (that's my opinion, not everyones, and I understand that :-) ) Having said that, I'd probably still say it's worth having some kind of guidelines to set expectations?
I'm still a tad confused/curious about v0.2 vs v1... For now I'm just hanging with v0.2 because it's what I know, but whenever I try to suggest Mithril to anyone I get awkward about the "there's a rewrite in progress, but don't worry" type discussion...
Is it worth considering Testling or Browserstack (etc) for tests?

dead-claudia commented 7 years ago

@lhorie

The only thing I disagree w/ is the line limit. Screens are really big these days and breaking lines mid-argument-list like in C++ is not super idiomatic (and personally, I find it difficult to read)

My screen can only comfortably fit about 120 monospace characters across (and it's a relatively open editor). Not all of us have larger screens, because some of us prefer smaller laptops. (I find larger laptops a little uncomfortable to use, because of my small hands.)

Several of the lines are upwards of 150 characters across, and it's harder to catch bugs at a glance when you have to scroll to see them.

I think I had the baseline at IE8 (assuming you're aware of XHR limitations)

I thought it was IE 9, since Microsoft's support cutoff of IE 8 and earlier was around the time you were making the decision for the baseline. (They only support IE 7 and 8 in a few very select legacy cases, and those IIRC will be dropped in 2017 or 2018.)

dead-claudia commented 7 years ago

@barneycarroll

@isiahmeadows' issue admin fatigue

:+1: :frowning:

It's so real, and that's the main reason why I've been looking for ways to streamline the high-level, less code-y aspect of contributing. It's much easier to help people contribute when I can point them in the right direction, and with the current state of things, it's not exactly helpful.

dead-claudia commented 7 years ago

@bruce-one

needs-tests tag sounds like a great idea

We have needs test case, and I already use it. I don't know how much @lhorie or @tivac use it, though.

Very rigid contribution guidelines are often a turnoff for me (but I understand them), my personal preference would be to preface the whole CONTRIBUTING.md with "When in doubt, make the PR, an incomplete PR is better than nothing!" (that's my opinion, not everyones, and I understand that :-) ) Having said that, I'd probably still say it's worth having some kind of guidelines to set expectations?

I agree that overly rigid ones can be a little problematic (and this project is thankfully small enough it doesn't need that kind of thing; it's not like Node.js or ESLint, with a large amount of contributors of varying frequency). But IMHO, something is better than nothing when it comes to guidelines. Style nits are super easy to fix, so I will simply politely point out the issues, but missing test cases make it harder to catch bugs in the future, and I'm not a fan of merging new features or changes to existing ones without tests additions/edits to match.

Also, if something breaks things, I've already learned the hard way what that can do (I'm only human). I know very few will actually look at Travis when the test fails, so I will usually try to point out where things went wrong (definitely if it's not their fault). 99% of the time, though, it's a simple linter issue or assertion failure because of their patch, and a simple local npm test could've caught it. And since there's no polite way to point out "Please run npm test locally to test your patch; it's broken", it's much harder to deal with the large number of frequently broken pull requests.

That last paragraph is why I really want a real, clean, and readable contributing guide that's easy to follow, and a code base that newcomers will have no trouble reading and understanding the basics of. It eventually gets tiring dealing with broken PRs due to not understanding the inner workings of the part in question, and I quite frankly don't have the time to keep up with that on the frequent basis it's been.

I will note that the prefilled issue template has been massively helpful. I'm no longer nearly as hard-pressed finding what the heck the actual problem is. @lhorie I'm still ever grateful for that, BTW.

I'm still a tad confused/curious about v0.2 vs v1

0.2 is the old, currently stable API. It's useful, but the abstractions are suboptimal, and it's a terrible mess internally. It's based on the MVC paradigm, giving you a way to specify the view and controller, leaving the model up to you to organize (idiomatically a plain JS object with m.props).
1.0 is the new, unstable API. It's a little more low-level and less magical, but it's much more flexible. You don't need nearly the indirection to do what you need, even when manipulating the DOM directly. It's component-based, down a similar vein as React. Instead of a controller, you have component state, but it's less opinionated on how you manage your state.

Is it worth considering Testling or Browserstack (etc) for tests?

I'll note that Testling is pretty much dead, and useless unless you're using Tape or similar (ospec doesn't support TAP output, at least yet). Beyond that, I don't have much of an opinion.

I'll note that it is possible to do some limited browser testing on Travis (install Chrome/Firefox on Linux or use Safari on OS X, run them with Karma and appropriate adapters/runners) and Appveyor (use Karma with pre-installed IE or Edge), and I already do on OS X and Linux with my Thallium test framework. Browserstack, Sauce Labs, etc. would be better, though.

barneycarroll commented 7 years ago

@isiahmeadows what do you think of the idea of a pull request npm script bundled in? This way we could advise potential contributors to make their changes and run npm run PR and

Run the tests
Bundle the scripts
Confirm desired feature branch
Push
Take you to the pull request page for your branch

I think that would make life more pleasant for contributors and admins

tivac commented 7 years ago

I think that sounds nice and also a bit of a PITA to get people to use.

orbitbot commented 7 years ago

Just throwing this out here, not that it necessarily should be considered: For the test before PR issue, you could always set up a precommit hook in git to block the commit if test runs fail. It's a bit of a hardline choice and won't stop anyone making PRs from the web interface, but that's probably infrequent in any case.

barneycarroll commented 7 years ago

@orbitbot blocking push on failing tests is a bad idea. Developing tests for a PR collaboratively is legit (often a formative educational experience), and developing failing tests collaboratively to properly identify tricky bugs or formalise feature acceptance criteria (TDD) is a thing in its own right. I suppose what I'd like is a hand-holding assistant to help you make your PR as complete as possible but in an optional non-imperative way, to guide & offer collaborators a procedural system that'll help them feel confident and shave of those cases where oversight / forgetfulness / ignorance / lack of confidence that would otherwise result in people giving up, or end in that horrible PR limbo where someone (typically @isiahmeadows) has to parse and grok PRs, determine simple failings, respond to author, hope he hasn't been too off-putting, wait for them to respond, etc. I feel his pain on that front.

But this is a pretty ambitious & experimental side project, probably best to iterate on a proof of concept it in isolation from Mithril. Let's kill this tangent — I'll post back when I've got a PoC repo set up and gather feedback.

supermensa commented 7 years ago

This is not a comment on the technical side, but a note regarding how newcomers might perceive Mithril.

I think that you should consider how Mithril is presented on the website. The Github readme is actually a lot cleaner and gets to the point.

The description of Mithril as "A Javascript Framework for Building Brilliant Applications" doesn't really tell me why I should start using it or why it's better than React or Angular 2 or whatever. Mithril could also be a brilliant death metal band from Norway.

Convince the newcomers why they should give it a try, then make it easy for them to come here and help make it even better.

barneycarroll commented 7 years ago

Mithril could also a brilliant death metal band from Norway

@supermensa a euphemism for 'inaccessible & slightly menacing'? ;P

lhorie commented 7 years ago

Mithril could also a brilliant death metal band from Norway

@supermensa I have a guitar and long hair, so I guess we're half way there :)

supermensa commented 7 years ago

Well, in the spirit of death metal - this blog post by James Forbes kills off Backbone.js just to hail Mithril: The Road from Backbone to Mithril

I think it has some good selling points that you could use:

mithril is faster than your custom application rendering logic

The entire point of mithril is to be small and fast.

If you know how to write fast JS, you already know how to use mithril.

That sounds like a brilliant framework

dead-claudia commented 7 years ago

I'll note that the current website objectively sucks, and we all know. We've just been too busy with working on the 1.x rewrite than redesigning a poorly done website.

dead-claudia commented 7 years ago

Closing, since this issue has not been updated in over a month, and typically, issues inactive for that long do not usually produce any action. If you feel something in Mithril needs added or changed, please file a new issue.

MithrilJS / mithril.js