docs: add CONTRIBUTING, DEVELOPING and VSCODE #977

bajtos · 2018-02-08T12:54:17Z

Start a guide for contributors in a place that's recognized by GitHub - add a short CONTRIBUTING.md file serving as a collection of links pointing to other documentation.

Start a guide for developers in DEVELOPING.md.

Describe VS Code setup in VSCODE.md, document steps needed to verify
integration with tsc and tslint.

Add docs/apidocs.html as an index file pointing to apidoc files of individual monorepo packages.

This work is loosely related to #950 and #574. Even though it is not in MVP scope, I think it's very important for us internally, to avoid regressions like the one introduced by #964

I am intentionally not wrapping lines at 80 characters, because this content will be rendered by GitHub and such line-breaks would be preserved in the rendered output.

The text is inspired by the following guides:

Node.js pull-requests.md and CONTRIBUTING.md
AngularJS DEVELOPERS.md

Checklist

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
Related API Documentation was updated
Affected artifact templates in packages/cli were updated
Affected example projects in packages/example-* were updated

bajtos · 2018-02-08T13:16:56Z

docs/CONTRIBUTING.md

+7. Open "Tasks" OUTPUT window and verify that compilation error was parsed by VSCode.
+
+8. Verify that compilation errors are correctly associated with the problematic source code line.
+   _(2018-02-82: this is does not work now, `tsc` is reporting paths relative to individual package directories.)_


FWIW: this used to work before, I suspect #682 broke it.

The trick is that we need to run tsc from the root monorepo directory.

https://github.com/strongloop/loopback-next/pull/682/files#diff-272a4137cf0e755785f43213ae0bae7fL14

Under the hood, we change the current directory to loopback-next root
and call tsc with project configuration from the original package's
tsconfig.build.json. This way tsc error messages contain a path
that's relative to loopback-next project root.

https://github.com/strongloop/loopback-next/pull/682/files#diff-272a4137cf0e755785f43213ae0bae7fL60

'-p', // It's important to keep this path relative to rootDir path.join(packageDir, 'tsconfig.build.json'),

I have some ideas how to fix this (basically introduce a new lb-tsc argument like -r ../.. that will control the working directory of tsc), but I prefer to leave such changes out of this documentation-only pull request.

virkt25

Good content to prevent regressions but the entire document seems very VSCode oriented and very intensive for any non-VSCode users (I was thinking of going back to SubLime :P).

Looking at most Open Source projects, CONTRIBUTING.md is a place to walk users through the setup of the repo locally (install, bootstrap, etc.) and commit guidelines, naming conventions, etc. I think we need to have a new doc for VSCODE.md or something. I don't like a project being so rigorous on maintaining editor config / compatibility because it will deter contributors who may not want to use VSCode / have VSCode.

I think this is something we can look for in Reviews and test ourselves and update if needed. A best practices / guidance doc for this content in a separate file is fine though. Just don't think it should be part of CONTRIBUTING.md.

bajtos · 2018-02-09T11:15:39Z

Thank you @virkt25 for a thoughtful comment. I fully agree with you
that we should not make the impression that loopback-next can be developed
only using VisualStudio Code.

I am a long time user of ViM which I still basically for everything except
loopback-next, so I understand your concerns.

As I was thinking about this a bit more, I agree with your that most of my
new content does not really belong to CONTRIBUTING.md. I am going to reorganize
it and let you know when I have new version to review.

bajtos · 2018-02-12T13:18:21Z

@virkt25 I reorganized the content and add more details, PTAL again.

@strongloop/sq-lb-apex @strongloop/loopback-next This pull request is starting CONTRIBUTING, DEVELOPERS and VSCODE guides, please take a look and let me know if you have any feedback.

I'd like to keep the scope of this pull request small please. IMO, an incomplete guide is better than no guide. Let's land something meaningful quickly and then we can incrementally add more content as needed.

bajtos · 2018-02-12T13:20:34Z

You can preview the new content here:

A screenshot of apidocs.html:

virkt25

Great job! I’m still reviewing. Posting this since it’s from my phone. Will go over rest on my laptop.

virkt25 · 2018-02-12T14:31:12Z

docs/CONTRIBUTING.md

@@ -0,0 +1,41 @@
+# Contributing to LoopBack
+
+Contributions to Node.js include code, documentation, answering user questions, and advocating for all types of Node.js users. See our official documentation on loopback.io for more information common to all of our GitHub repositories:


Contributing to LoopBack!

virkt25 · 2018-02-12T14:37:29Z

docs/CONTRIBUTING.md

+## [Contributing code](http://loopback.io/doc/en/contrib/code-contrib.html)
+
+Pull Requests are the way concrete changes are made to the code, documentation
+and tools contained in LoopBack repositories.


This is on a new line

dhmlau

It looks good!! Just a few suggestions.

dhmlau · 2018-02-12T16:02:32Z

docs/CONTRIBUTING.md

+ - [Using Jekyll](http://loopback.io/doc/en/contrib/jekyll_getting_started.html)
+ - [Authoring pages](http://loopback.io/doc/en/contrib/pages.html)
+ - [Translations](http://loopback.io/doc/en/contrib/translation.html)
+


I'm thinking we might need to add the commit message guidelines to here as well.
e.g. https://loopback.io/doc/en/contrib/git-commit-messages.html
There were quite a number of doc PRs at the end of last year that failed in the commit linter.

LoopBack Next uses different rules for commit messages and @bajtos has included details for that in DEVELOPING.md.

If the doc PR is in loopback.io, I think the commit message guideline from https://loopback.io/doc/en/contrib/git-commit-messages.html is still valid.
But yes, if it's from one of the READMEs, then it will follow the one in loopback-next.

I am going to add a link to that other page too.

[Git commit messages](./DEVELOPING.md#commit-message-guidelines) (Note that other repositories like [strongloop/loopback.io](https://github.com/strongloop/loopback.io) use a different [convention](https://loopback.io/doc/en/contrib/git-commit-messages.html).)

On the second thought, I am going to revert the change mentioned in my previous comment and move the link to loopback.io commit message guidelines to the actual text in DEVELOPING.md file. A new commit will come shortly.

dhmlau · 2018-02-12T16:07:54Z

docs/CONTRIBUTING.md

+Pull Requests are the way concrete changes are made to the code, documentation
+and tools contained in LoopBack repositories.
+
+ - [Setting up development environment](./DEVELOPING.md#setting-up-development-environment)


Since most of the links are coming from DEVELOPING.md, I wonder if we could group them to a category sth like "what you need to know to develop LB". It might be more convenient to the users to go to that link and read rather than going to each link and basically pointing to the same page.
Maybe it's just me...

I actually like the separate links (even though they are all pointing to the same page).

Here is my point of view:

"what you need to know to develop LB" is already grouped in DEVELOPING.md. Users wishing to read everything about DEVELOPING in one go can already read that file!

I could shorten this list and replace all items pointing to different sections of DEVELOPING with a single link pointing to that file, but I think that's worse user experience because users cannot quickly jump from CONTRIBUTING to a relevant section in DEVELOPING.

I see two kinds of users:

Jane would like to learn everything about developing loopback-next in one go. She can open DEVELOPING file and read through it.

Joe prefers to jump to the code and read only as much as is needed. Links in CONTRIBUTING makes it easier for him to see what different topics are available and choose to read only what's relevant for him.

FWIW, Node.js uses similar approach in their CONTRIBUTING.md

virkt25

Great job!

virkt25 · 2018-02-12T18:09:29Z

docs/DEVELOPING.md

+we recommend to do a full reinstall of all npm dependencies:
+
+```sh
+$ npm run clean:lerna && npm run bootstrap


I think running just clean with bootstrap is fine. npm run clean && npm run bootstrap. Any reason to use clean:lerna here?

clean:lerna removes node_modules files in all package folders (something like rm -rf packages/*/node_modules).

I vaguely remember having issues with dependency setup that were resolved by removing all node_modules folders.

If this is no longer a problem, then I am happy to change this line as you suggested.

@raymondfeng could you please chime in? AFAICT, clean:lerna was added by you in f1e59a2, which says:

This change fixes the issue that npm run release fails if a new package is added to the monorepo before bootstrap is done. The new package doesn't have dependencies installed yet.

Can we safely assume that clean:lerna script is needed only for the release process?

clean:lerna is mostly for release but it is also helpful in certain situations to force a fresh bootstrap.

richardpringle · 2018-02-12T20:40:17Z

docs/CONTRIBUTING.md

+Pull Requests are the way concrete changes are made to the code, documentation
+and tools contained in LoopBack repositories.
+
+ - [Setting up development environment](./DEVELOPING.md#setting-up-development-environment)


I actually like the separate links (even though they are all pointing to the same page).

richardpringle · 2018-02-12T20:41:23Z

docs/DEVELOPING.md

+TypeScript too.
+
+ - [VisualStudio Code](./VSCODE.md)
+ - _Missing your favorite IDE/editor here? Pull requests are welcome!_


On the second thought, I am going to rewrite and expand the sentence Pull requests are welcome!, it has built negative connotations over the years.

richardpringle · 2018-02-12T20:49:02Z

docs/DEVELOPING.md

+
+#### scope
+
+The **scope** must be a list of one or more packages contained in this monorepo. Eeach scope name must match a directory name in [packages/](../packages), e.g. `core` or `context`.


Eeach

woops

bajtos · 2018-02-15T09:14:43Z

Thank you @virkt25 @dhmlau and @richardpringle for your comments!

I have rebase this patch on top of the latest master (50cfbcc) and addressed your comments in 3b72ced.

On the second thought, I decided to move CONTRIBUTING.md to the root directory of our repository (d2fbc8f), to give it a more prominent place making it easier to discover by people looking around our GitHub repo. (This is what Node.js project does too, FWIW).

LGTY now?

bajtos · 2018-02-15T09:17:28Z

New preview links:

b-admike · 2018-02-15T13:44:05Z

docs/VSCODE.md

+7. Open "Tasks" OUTPUT window and verify that compilation error was parsed by VSCode.
+
+8. Verify that compilation errors are correctly associated with the problematic source code line.
+   _(2018-02-82: this is does not work now, `tsc` is reporting paths relative to individual package directories.)_


typo if 2018-02-82 is a date here. I suggest we link it to an issue, but if it's fixed, we can get rid of this message.

Will fix. I opened #1010 to keep track of this bug.

b-admike · 2018-02-15T13:45:10Z

docs/VSCODE.md

+
+#### Refactoring in VS Code
+
+Verify that refactorings like "rename" will change all places using the renamed entity. Two different scenarios to verify: rename at the place where the entity is defined, rename at the place where the entity is used. (You can e.g. rename `inject` to test.)


I think for beginners saying rename symbol or F2 might be better than rename.

raymondfeng · 2018-02-15T21:36:44Z

docs/apidocs.html

+      <h2>LoopBack4 packages (local)</h2>
+
+      <p>Run the following command to (re)build API doc files:
+      <p><pre><code>$ lerna run build:apidocs</code></pre>


We probably should use npx lerna run build:apidocs or add a script to top level package.json.

Good catch! I have ./node_modules/.bin in my PATH and my Bash configured to run npx for commands not found, but I agree that's not something we can rely on in our docs.

raymondfeng · 2018-02-15T22:29:45Z

FYI : #1005. Feel free to reference it.

Add a short CONTRIBUTING.md file serving as a collection of links pointing to other documentation. Start a guide for developers in DEVELOPING.md. Describe VS Code setup in VSCODE.md, document steps needed to verify integration with tsc and tslint. Add docs/apidocs.html as an index file pointing to apidoc files of individual monorepo packages.

bajtos · 2018-02-16T08:56:52Z

FYI : #1005. Feel free to reference it.

I'll open a new pull request for that, I really want to get this one merged - it has been in progress for too long to my taste.

bajtos added this to the February 2018 milestone Feb 8, 2018

bajtos self-assigned this Feb 8, 2018

bajtos requested review from raymondfeng, kjdelisle and virkt25 February 8, 2018 12:54

bajtos added the needs-priority label Feb 8, 2018

bajtos mentioned this pull request Feb 8, 2018

Update Contrib docs to match the new reality #950

Closed

bajtos commented Feb 8, 2018

View reviewed changes

virkt25 reviewed Feb 8, 2018

View reviewed changes

bajtos force-pushed the docs/describe-how-to-test-tsc-tslint-changes branch 4 times, most recently from 18d77ca to 72b0bc5 Compare February 12, 2018 13:12

bajtos changed the title ~~docs: describe tsc/tslint verification in CONTRIBUTING.md~~ docs: add CONTRIBUTING, DEVELOPING and VSCODE Feb 12, 2018

virkt25 reviewed Feb 12, 2018

View reviewed changes

dhmlau reviewed Feb 12, 2018

View reviewed changes

virkt25 reviewed Feb 12, 2018

View reviewed changes

richardpringle suggested changes Feb 12, 2018

View reviewed changes

bajtos mentioned this pull request Feb 13, 2018

doc: Move contribution doc from wiki to repo #574

Closed

bajtos force-pushed the docs/describe-how-to-test-tsc-tslint-changes branch from 72b0bc5 to 3b72ced Compare February 15, 2018 09:11

b-admike reviewed Feb 15, 2018

View reviewed changes

dhmlau approved these changes Feb 15, 2018

View reviewed changes

richardpringle approved these changes Feb 15, 2018

View reviewed changes

raymondfeng reviewed Feb 15, 2018

View reviewed changes

bajtos mentioned this pull request Feb 16, 2018

Fix tsc setup to report project-relative paths in error messages #1010

Closed

2 tasks

bajtos force-pushed the docs/describe-how-to-test-tsc-tslint-changes branch from 34ec48f to 33c93fa Compare February 16, 2018 08:56

bajtos merged commit 7a4545e into master Feb 16, 2018

bajtos deleted the docs/describe-how-to-test-tsc-tslint-changes branch February 16, 2018 09:30

bajtos removed the needs-priority label Feb 16, 2018

bajtos mentioned this pull request Feb 19, 2018

Improve developer docs #1015

Merged

6 tasks

mrmodise mentioned this pull request Dec 29, 2020

docs: simplify middleware-based sequence #6824

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: add CONTRIBUTING, DEVELOPING and VSCODE #977

docs: add CONTRIBUTING, DEVELOPING and VSCODE #977

bajtos commented Feb 8, 2018 •

edited

Loading

bajtos Feb 8, 2018

virkt25 left a comment

bajtos commented Feb 9, 2018

bajtos commented Feb 12, 2018

bajtos commented Feb 12, 2018

virkt25 left a comment

virkt25 Feb 12, 2018

virkt25 Feb 12, 2018

dhmlau left a comment

dhmlau Feb 12, 2018

virkt25 Feb 12, 2018

dhmlau Feb 12, 2018

bajtos Feb 15, 2018

bajtos Feb 15, 2018

dhmlau Feb 12, 2018

richardpringle Feb 12, 2018

bajtos Feb 15, 2018

virkt25 left a comment

virkt25 Feb 12, 2018

bajtos Feb 15, 2018

raymondfeng Feb 15, 2018 •

edited

Loading

richardpringle Feb 12, 2018

richardpringle Feb 12, 2018

bajtos Feb 15, 2018

richardpringle Feb 12, 2018

bajtos commented Feb 15, 2018

bajtos commented Feb 15, 2018

b-admike Feb 15, 2018

bajtos Feb 16, 2018

b-admike Feb 15, 2018

raymondfeng Feb 15, 2018

bajtos Feb 16, 2018

raymondfeng commented Feb 15, 2018

bajtos commented Feb 16, 2018 •

edited

Loading

		@@ -0,0 +1,41 @@
		# Contributing to LoopBack

		Contributions to Node.js include code, documentation, answering user questions, and advocating for all types of Node.js users. See our official documentation on loopback.io for more information common to all of our GitHub repositories:


		#### scope

		The scope must be a list of one or more packages contained in this monorepo. Eeach scope name must match a directory name in [packages/](../packages), e.g. `core` or `context`.


		#### Refactoring in VS Code

		Verify that refactorings like "rename" will change all places using the renamed entity. Two different scenarios to verify: rename at the place where the entity is defined, rename at the place where the entity is used. (You can e.g. rename `inject` to test.)

docs: add CONTRIBUTING, DEVELOPING and VSCODE #977

docs: add CONTRIBUTING, DEVELOPING and VSCODE #977

Conversation

bajtos commented Feb 8, 2018 • edited Loading

Checklist

Choose a reason for hiding this comment

virkt25 left a comment

Choose a reason for hiding this comment

bajtos commented Feb 9, 2018

bajtos commented Feb 12, 2018

bajtos commented Feb 12, 2018

virkt25 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhmlau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

virkt25 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Feb 15, 2018

bajtos commented Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng commented Feb 15, 2018

bajtos commented Feb 16, 2018 • edited Loading

bajtos commented Feb 8, 2018 •

edited

Loading

raymondfeng Feb 15, 2018 •

edited

Loading

bajtos commented Feb 16, 2018 •

edited

Loading