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

Development.md: How to Develop Meteor Itself #8267

Merged
merged 35 commits into from Mar 9, 2017

Conversation

abernix
Copy link
Contributor

@abernix abernix commented Jan 24, 2017

The current edition of Contributing.md is great in that it's beginning to offer a lot of information on the various ways to contribute to Meteor aside from just code contributions including Issue Triaging, PR help, etc.

It falls a bit short when it comes to actual development though as there is also quite a bit that is undocumented in that aspect. There's a bit of a section on running Meteor tests, but aside from the "Slow start for developers" section of the README.md there isn't much breaking down the nitty gritty – what a dev bundle is, how you run specific tests, style expectations, etc. These might be better documented in a Development.md (or any other name we believe is reasonable) which would specifically be for developers making code contributions to the repo and would assist in helping users actually running Meteor from a checkout.

The initial straw-man proposal here is:

  • Move the "Running Meteor tests" section to this new document.
  • Move the "Slow start for developers" section to this new document
  • Elaborate on different ways to run the tests
  • Explain how the current CI runs tests
  • Make it clear that tests don't work on Windows
  • Briefly provide links to different parts of the Meteor Build system which have documentation hiding in the code/repo (for example the README files of the various packages, or the Isobuild README)
  • Add information on how to run your tests on your own CircleCI
  • Add information about the style expectations for new code

Contributing.md can then gain some additional information like where to find work:

  • Show pull-requests-encouraged
  • Mention some "first contribution ideas" (style?)

There are a number of community contributors (myself included) who I think might have some great perspective on this , so I'm tempted to start a PR with a blank Development.md and then (EDIT: I've done this, PRs welcome!) and encourage contributors to submit a PR against that PR so we can work through it.

Then again, if we just want to gather all the suggestions here and I can try to go at it in one caffeine-induced pass.

Thoughts? What are we missing to make it easier to contribute here.

/cc Current issue triagers and recent/frequent contributors: @hwillson @mitar @laosb @dburles @merlinpatt @sebakerckhof @GeoffreyBooth @lucfranken

Inspired by:

@hwillson
Copy link
Contributor

This would be awesome!

  • Development.md sounds like a good name to me;
  • The initial proposal looks great;
  • a PR with a blank Development.md sounds like a great approach.

Let's do this! Thanks for kicking this off @abernix!

@sebakerckhof
Copy link
Contributor

sebakerckhof commented Jan 18, 2017

Good idea, this would have helped me a lot in the past.

Although a bit outdated with the move to NPM. As the developer of https://github.com/sebakerckhof/stratosphere (private meteor package server) I could write a secton on how the meteor package system works.

@abernix
Copy link
Contributor Author

abernix commented Jan 18, 2017

@klaussner I should have probably CC'd you too – but glad you're paying attention. :)

@sebakerckhof Anything specifically or ideas you'd like to see here?

@GeoffreyBooth
Copy link
Contributor

Yes, I concur. Figuring out how to run the tests was a bit of a learning curve for me. This would’ve helped a lot.

@ericfortis
Copy link

What do you think about code walkthroughs videos?

@abernix
Copy link
Contributor Author

abernix commented Jan 19, 2017

@e50 I think it would be great if someone could pull something like that off at a Meteor Meetup or something (and then get it online), but I'd say it's too much to include in this issue; I think searchable documentation is the first goal. Once this is done, it might encourage something like that to happen and perhaps snowballing contribution effects will ensue. 😉

@hwillson
Copy link
Contributor

And just to add to @abernix's last comment - videos are really hard to keep up to date; text is MUCH easier to change.

@mitar
Copy link
Contributor

mitar commented Jan 19, 2017

Even screenshots is hard to keep up to date. It would be so great if there would be some service which would re-render at every version release based on your interaction with the mouse with your app. :-)

@abernix
Copy link
Contributor Author

abernix commented Jan 24, 2017

I've gone ahead and converted this to a PR per the above conversation. PRs against this branch (feature/8267-development-dot-md) are welcome now, but I'll try to make an initial pass to knock out a few bullet-points on this in the next few days! Glad to see the fanfare so far!

@abernix abernix changed the title Create a Development.md [WIP] Create a Development.md Jan 25, 2017
@abernix abernix added the in-development We are already working on it label Jan 25, 2017
@abernix
Copy link
Contributor Author

abernix commented Jan 30, 2017

/cc @skirunman

@@ -39,6 +47,7 @@ Current Issue Triagers:
Our most regular and experienced Issue Triagers sometimes move on to doing code reviews for pull requests, and have input into which pull requests should be merged.

Current Reviewers:
- [@hwillson](https://github.com/hwillson)
- [@lorensr](https://github.com/lorensr)
- [@abernix](https://github.com/abernix)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still accurate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, going to go with this for now. I think this allows users to easily know who to reach out to for help, and we've added @hwillson who has been a tremendous help lately.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding me guys - I'm definitely looking forward to helping out more! (well, that is unless of course an Amazon S3 outage completely ruins my day and leads to me leaving tech completely ...)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be super clear @hwillson I was not questioning your addition, just whether Jesse meant to leave himself here when he added himself to another section.

Copy link
Contributor

@stubailo stubailo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Just a few small comments.

Contributing.md Outdated
- [Reviewing pull requests](https://github.com/meteor/meteor/blob/devel/Contributing.md#reviewer)
- [Maintaining a community package](https://github.com/meteor/meteor/blob/devel/Contributing.md#community-package-maintainer)

There are also several ways to contribute to the Meteor Project outside of GitHub, like organizing or speaking at [Meetups](https://www.meetup.com/topics/meteor/) and events and helping to moderate our [forums](https://forums.meteor.com/). Stay tuned for more documentation around non-code contributions.

If you can think of any changes to the project or documentation that would improve the contributor experience, let us know by opening an issue!
If you can think of any changes to the project or [documentation](https://github.com/meteor/docs) that would improve the contributor experience, let us know by opening an issue!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or guide!

Development.md Outdated
>
> $ git submodule update --init --recursive

0. **_(Optional)_ Compile dependencies**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've almost never had to do this in 2 years of working on Meteor - I'd put this in a separate section or something - maybe called "modifying the dev bundle"

Copy link
Contributor Author

@abernix abernix Mar 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do this quite often, but you're right that it should be in a separate section.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a new section for modifying the dev bundle!



```sh
$ ./meteor --help
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe --get-ready is better?

Copy link
Contributor Author

@abernix abernix Mar 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was intentional. meteor --get-ready actually does quite a bit more compilation and takes about 5x longer.

I guess I should document exactly what compilation it does (but somewhere else).


```sh
$ cd my-app/
$ /path/to/meteor-checkout/meteor run
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest making an alias for people who are planning on doing a lot of development.

Per PR suggestions, add a suggestion to make an alias for more frequent use.  Also, due to difficulty of formatting aforementioned change, move unrelated Note (which didn't belong there) to a more prominent section called "Notes when running from a checkout"
@abernix abernix changed the title [WIP] Create a Development.md Create a Development.md Mar 1, 2017
This moves the dev bundle step out of the "Running from checkout", simplifying those steps, but breaks things down much more in its own, new, section.
@abernix abernix changed the title Create a Development.md Development.md: How to Develop Meteor Itself Mar 2, 2017
Development.md Outdated

## Notes when running from a checkout

The following are some distict differences you must pay attention to when running Meteor from a checkout:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small typo - "distict" should be "distinct".

Development.md Outdated

./meteor test-packages

Exactly in the same way that [`test-packages` works in standalone Meteor apps](https://guide.meteor.com/writing-atmosphere-packages.html#testing), the `test-packages` command will start up a Meteor app with [TinyTest](./packages/tinytest/README.md). To view the results, just connect to `http://localhost:3000` (or your specified port) and view the results.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can remove the "and view the results." part of the last sentence (since "To view the results" is already mentioned at the beginning).

Development.md Outdated

While TinyTest and the `test-packages` command can be used to test internal Meteor packages, they cannot be used to test the Meteor Tool itself. The Meteor Tool is a node app that uses a home-grown "self test" system.

For even more details on how to run Meteor Tool "self tests", please refer to the [Testing section of the Meteor Tool README](https://github.com/meteor/meteor/blob/master/tools/README.md#testing).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might makes sense to move this sentence to the end of the "Running Meteor Tool self-tests" section. It seems a bit misplaced here, as we just started talking about self-test then immediately say "for even more details ...", but we haven't really given any details yet. If placed at the end of this section, then we would have.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would agree, but it gets lost in the "Excluding specific tests". I could add a "### More reading" section and put it there?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point and great idea - a more reading section would help.

Development.md Outdated

### Continuous integration

Any time a pull-request is submitted or a commit is pushed directly to the `devel` branch, continuous integration tests will be started automatically by the CI server. These are run by [Circle CI](https://circleci.com/) and defined in the [`circle.yml` file](./circle.yml) file. Even more specifically, the tests to run and the containers to run them under are defined in the [`/scripts/ci.sh`](scripts/ci.sh) script, which is a script which can run locally to replicate the exact tests.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The word file is duplicated - "circle.yml file file".

Development.md Outdated
When `meteor` is run from a checkout, a `dev_bundle` is automatically downloaded and should be sufficient for most development. However, some more substantial changes will require rebuilding the `dev_bundle`. This include changes to the:

* Node version
* NPM version
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The official spelling is "npm", in lowercase (also in line 79).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why we capitalize anything, anywhere. (Fixed, Thank you!)

@hwillson
Copy link
Contributor

hwillson commented Mar 2, 2017

This all looks great @abernix - I added a few small comments regarding typo's/structuring, but this looks like it's good to go!

The only additional details I can think to add that might be helpful for developers, are a few tips on how to use git effectively when working on the core. Things like how to keep your clone in synch with the meteor repo, how to keep your branch up to date with devel, recommendations on squashing commits (to do or not to do, until a PR is about to be merged for example), etc. That being said, this really is more general git help, so maybe we don't want to go there. Given the number of recent PR's that have been opened by mistake or include commits that don't belong however, maybe it's worth considering.

Thanks to @klaussner and @hwillson for these suggestions!  I also changed some other trademark-y things. 😉
@abernix
Copy link
Contributor Author

abernix commented Mar 2, 2017

That's a very reasonable suggestion and it's probably a good idea. There are a lot of great developers who don't (or have never used) Git.

My hunch is that we could find some awesome tutorials that other projects link to. I'm going to vote for getting this done (as is) for now and adding that as soon as someone can work on it. The next phase of this is to find ways to hook developers into it, so we do want to make sure they're ready when they get here!

@@ -0,0 +1,196 @@
# Development
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a brief summary here about the purpose of this document? Otherwise it's two headers with nothing in between.

Copy link
Contributor

@benjamn benjamn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great improvement to our documentation!

This was inadvertently set as a "H1" (`#`) but should have been nested within the "Running from a Git checkout" section as an "H2" (`##`).
@abernix abernix merged commit 7e36188 into devel Mar 9, 2017
@abernix
Copy link
Contributor Author

abernix commented Mar 9, 2017

Squashed!

A huge "thanks" to everyone who helped review this or made suggestions initially, and since this issue is mostly followed by those who help triage, maintain positive direction in the project or make high-quality contributions regularly, for all of your assistance! 👏

(Also, a proactive "thank you" to anyone who uses these tips to make contributions in the future! Anyone looking for a place to test these instructions, these issues are ready to go 😄)

If anything else comes to mind for these either of these docs, feel free to make a pull-request! It's as easy as clicking "Edit" on GitHub. 😉

abernix added a commit that referenced this pull request Mar 10, 2017
Many aspects regarding contribution of code can be better explained in the new `Development.md` so provide links to those relevant parts.

Follows up on #8267
@ghost ghost mentioned this pull request Mar 15, 2017
@abernix abernix deleted the feature/8267-development-dot-md branch April 17, 2017 04:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-development We are already working on it Type:Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants