feat(getting-started): migrate into monorepo #869

kjdelisle · 2018-01-16T16:37:10Z

The loopback4-example-getting-started repository is now being moved into the monorepo

blocks #727
implements a part of #836

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

kjdelisle · 2018-01-16T18:31:01Z

@slnode test please

raymondfeng · 2018-01-17T15:42:23Z

packages/getting-started/LICENSE

@@ -0,0 +1,25 @@
+Copyright (c) IBM Corp. 2017. All Rights Reserved.


Technically, this is a migration of existing licensed code; should it change to 2018, or be 2017-2018?

raymondfeng · 2018-01-17T15:43:28Z

packages/getting-started/LICENSE

@@ -0,0 +1,25 @@
+Copyright (c) IBM Corp. 2017. All Rights Reserved.
+Node module: @loopback/getting-started


Can we name the package as @loopback/example-getting-started to be consistent with @loopback/example-codehub?

raymondfeng · 2018-01-17T15:44:28Z

packages/getting-started/package.json

+  "engines": {
+    "node": ">=8"
+  },
+  "scripts": {


Can we add "private" : true, so that the module won't be published to npmjs? See https://github.com/strongloop/loopback-next/blob/master/packages/example-codehub/package.json#L8

I think we want this one to be published to npmjs, so that people can install it as a ready-made example, don't we?

virkt25 · 2018-01-17T18:18:47Z

packages/example-getting-started/LICENSE

@@ -0,0 +1,25 @@
+Copyright (c) IBM Corp. 2017-2018. All Rights Reserved.


Copyright years are separated by a comma. So 2017,2018.

virkt25 · 2018-01-17T18:19:16Z

packages/example-getting-started/index.js

@@ -0,0 +1,14 @@
+// Copyright IBM Corp. 2017. All Rights Reserved.


2017, 2018.

virkt25 · 2018-01-17T18:19:32Z

packages/example-getting-started/index.js

@@ -0,0 +1,14 @@
+// Copyright IBM Corp. 2017. All Rights Reserved.
+// Node module: loopback4-example-getting-started


Module name: @loopback/example-getting-started
year: 2017, 2018

virkt25 · 2018-01-17T18:20:35Z

packages/example-getting-started/src/application.ts

@@ -0,0 +1,57 @@
+import {Application, ApplicationConfig} from '@loopback/core';


Shouldn't there a a copyright header here?

virkt25 · 2018-01-17T18:20:45Z

packages/example-getting-started/src/controllers/index.ts

@@ -0,0 +1 @@
+export * from './todo.controller';


Copyright header?

virkt25 · 2018-01-17T18:22:11Z

packages/example-getting-started/src/repositories/todo.repository.ts

@@ -0,0 +1,12 @@
+import {DefaultCrudRepository, DataSourceType} from '@loopback/repository';


copyright header?

virkt25 · 2018-01-17T18:22:19Z

packages/example-getting-started/test/acceptance/application.test.ts

@@ -0,0 +1,112 @@
+import {createClientForHandler, expect, supertest} from '@loopback/testlab';


copyright header?

virkt25 · 2018-01-17T18:22:30Z

packages/example-getting-started/test/helpers.ts

@@ -0,0 +1,36 @@
+import {Todo} from '../src/models/index';


copyright header?

virkt25 · 2018-01-17T18:22:37Z

packages/example-getting-started/test/unit/controllers/todo.controller.test.ts

@@ -0,0 +1,125 @@
+import {expect} from '@loopback/testlab';


copyright header?

virkt25 · 2018-01-17T18:23:33Z

packages/example-getting-started/.gitignore

@@ -0,0 +1,3 @@
+*.tgz


This file isn't needed as it's covered by top-level .gitignore.

virkt25

I know the tool picked the year as 2018 ... but like you said, since this was a move, shouldn't it be 2017,2018.

If not then change LICENSE to be consistent and just 2018 as well.

virkt25 · 2018-01-17T19:17:55Z

packages/example-getting-started/README.md

+## Setup
+1. Clone this repository if you haven't already:
+```
+git clone https://github.com/strongloop/loopback4-example-getting-started


This should be updated to use the new lb4 example getting-started command ... and it should be enabled in the CLI.

virkt25 · 2018-01-17T21:58:02Z

packages/example-getting-started/README.md

+## Setup
+1. Clone this repository if you haven't already:
+```
+git clone https://github.com/strongloop/loopback-next


Update this to use lb4 example getting-started instead of cloning the mono-repo. You should be good after this change.

IMO, the correct instructions are along the following lines:

npm install -g loopback4-cli

lb4 example getting-started

cd loopback4-example-getting-started

npm install

npm start

bajtos · 2018-01-18T08:38:03Z

Can we add "private" : true, so that the module won't be published to npmjs? See https://github.com/strongloop/loopback-next/blob/master/packages/example-codehub/package.json#L8

I think we want this one to be published to npmjs, so that people can install it as a ready-made example, don't we?

I don't think we want people to install this from npm. npm install @loopback/example-getting-started will put the example package to node_modules/@loopback/example-getting-started and will not install any dev dependencies. People should use lb4 example getting-started to get a local copy of this example.

bajtos

This is great! I found few places to improve, but I am ok with leaving those improvements for a follow-up pull request if it makes your work easier.

Once this PR is landed, we need to update https://github.com/strongloop/loopback4-example-getting-started - remove all files except README, shorten README to a link pointing people to this new location. Please don't forget to cross-link the pull requests!

bajtos · 2018-01-18T08:39:33Z

packages/cli/generators/example/index.js

@@ -13,6 +13,8 @@ const utils = require('../../lib/utils');

 const EXAMPLES = {
  codehub: 'A GitHub-like application we used to use to model LB4 API.',
+  'getting-started':
+    'A basic Todo application used as an introduction to LoopBack 4.',


Ideally, this description should match the description in package.json. See https://github.com/strongloop/loopback-next/pull/869/files#diff-27ee36b5a6726e3a1fa485380948168aR4:

"description": "An application and tutorial on how to build with LoopBack 4.",

bajtos · 2018-01-18T08:41:03Z

packages/example-getting-started/README.md

+
+Before we can begin, you'll need to make sure you have some things installed:
+- [Node.js](https://nodejs.org/en/) at v6.x or greater
+- [TypeScript](http://www.typescriptlang.org/)


A global installation of TypeScript should not be needed, typescript should be a dev-dependency of this package.

I know this is probably out of scope of this PR, but since we have to update the instructions anyways (see below), then I think it's best to fix this part too.

@kjdelisle PTAL ☝️ this hasn't been addressed yet.

bajtos · 2018-01-18T08:42:51Z

packages/example-getting-started/README.md

+Before we can begin, you'll need to make sure you have some things installed:
+- [Node.js](https://nodejs.org/en/) at v6.x or greater
+- [TypeScript](http://www.typescriptlang.org/)
+- [Git](https://git-scm.com/)


Git should not be needed, because people are going to obtain this example via lb4 example command.

Please update the instructions accordingly.

@kjdelisle PTAL ☝️ this hasn't been addressed yet.

bajtos · 2018-01-18T08:44:21Z

packages/example-getting-started/README.md

+## Setup
+1. Clone this repository if you haven't already:
+```
+git clone https://github.com/strongloop/loopback-next


IMO, the correct instructions are along the following lines:

npm install -g loopback4-cli

lb4 example getting-started

cd loopback4-example-getting-started

npm install

npm start

bajtos · 2018-01-18T08:47:13Z

packages/example-getting-started/package.json

+    "node": ">=8"
+  },
+  "scripts": {
+    "acceptance": "lb-dist mocha --opts ../../test/mocha.opts 'DIST/test/acceptance/**/*.js'",


FWIW, --opts ../../test/mocha.opts will not work after this example project is cloned via lb4 example command. We need to fix lb4 example to extract monorepo-level mocha.opts into test/mocha.opts and fix all package.json scripts referring to mocha.opts. Since test/mocha.opts is the default location loaded by Mocha, we should simply remove --opts argument completely.

Feel free to leave this fix out of scope of this PR, I think this may be easier to test once the master branch contains an example project using mocha.opts.

On the second thought, since we are already keeping almost everything build/test related in @loopback/build, it will be easier to move mocha.opts to that package too. See #876.

If/when #876 is landed, then this line needs to be changed to --opts node_modules/@loopback/build/mocha.opts

FYI: #876 has been landed, package scripts will need an update after this feature branch is rebased on top of the new master.

bajtos · 2018-01-18T08:48:25Z

packages/example-getting-started/package.json

+    "pretest": "npm run build:current",
+    "test": "lb-dist mocha --opts ../../test/mocha.opts 'DIST/test/unit/**/*.js' 'DIST/test/acceptance/**/*.js'",
+    "unit": "lb-dist mocha --opts ../../test/mocha.opts 'DIST/test/unit/**/*.js'",
+    "verify": "npm pack && tar xf loopback-getting-started*.tgz && tree package && npm run clean",


loopback-getting-started*.tgz will not exist, because the package name is @loopback/example-getting-started. I think we can afford to simplify the wildcard to something like tar xf *getting-started-*.tgz

bajtos · 2018-01-18T08:49:32Z

packages/example-getting-started/package.json

+    "example",
+    "tutorial"
+  ],
+  "private": true


Would you mind moving the private flag closer to the top of the file, e.g. just after version or description field? I think this is a pretty important flag, and when it's specified at the bottom of the file, then it's easy to miss when reading package.json.

bajtos · 2018-01-19T10:48:40Z

packages/example-getting-started/src/controllers/todo.controller.ts

+
+export class TodoController {
+  constructor(
+    @inject('repositories.TodoRepository')


This should be @repository(TodoRepository.name) to better support refactor-rename operations. See #744

bajtos

☹️

bajtos · 2018-01-19T10:50:11Z

packages/example-getting-started/src/controllers/todo.controller.ts

+
+  @post('/todo')
+  async create(@param.body('obj') obj: Todo): Promise<Todo> {
+    return await this.todoRepository.create(obj);


Yesterday you mentioned that this pull request is not only copying over files from loopback4-example-getting-started repository, but also making changes in the actual implementation. That's really bad, how are we supposed to find the differences and review those changes?

The code in the original repository contains many important TODO comments that we need to address for our MVP milestone.

In this particular place, the original createTodo method has additional hand-written validation as a workaround for missing #118. This validation should be preserved, or at least we should discuss whether and how we want to change this implementation bit.

Please update this pull request to copy as many source files in verbatim as is possible. Additional changes/updates/refactorings should be made in follow-up pull requests, only once the current codebase has been added to monorepo with as little modifications as possible.

Updated to incorporate feedback

The loopback4-example-getting-started repository is now being moved into the monorepo

kjdelisle · 2018-01-19T18:31:26Z

@slnode test please

kjdelisle requested review from bajtos and raymondfeng as code owners January 16, 2018 16:37

kjdelisle added the needs-priority label Jan 16, 2018

kjdelisle force-pushed the getting-started branch 5 times, most recently from a66ce45 to dd14e25 Compare January 16, 2018 18:15

raymondfeng reviewed Jan 17, 2018

View reviewed changes

kjdelisle force-pushed the getting-started branch 4 times, most recently from 5633a79 to f5ef471 Compare January 17, 2018 18:20

virkt25 suggested changes Jan 17, 2018

View reviewed changes

kjdelisle force-pushed the getting-started branch from 864ccbc to cf3dfb8 Compare January 17, 2018 19:19

virkt25 suggested changes Jan 17, 2018

View reviewed changes

kjdelisle force-pushed the getting-started branch from a389a10 to 66cc62a Compare January 17, 2018 19:45

shimks approved these changes Jan 17, 2018

View reviewed changes

raymondfeng approved these changes Jan 17, 2018

View reviewed changes

kjdelisle force-pushed the getting-started branch from a0b1b20 to 007de04 Compare January 17, 2018 20:44

virkt25 reviewed Jan 17, 2018

View reviewed changes

bajtos approved these changes Jan 18, 2018

View reviewed changes

virkt25 mentioned this pull request Jan 18, 2018

Bring examples back into the monorepo #836

Closed

12 tasks

kjdelisle force-pushed the getting-started branch from 007de04 to 08f40ea Compare January 18, 2018 17:57

bajtos reviewed Jan 19, 2018

View reviewed changes

bajtos previously requested changes Jan 19, 2018

View reviewed changes

kjdelisle force-pushed the getting-started branch 2 times, most recently from 814d4f5 to c69d52b Compare January 19, 2018 17:47

kjdelisle force-pushed the getting-started branch from c69d52b to bbb3af5 Compare January 19, 2018 18:14

virkt25 approved these changes Jan 19, 2018

View reviewed changes

feat(example-getting-started): migrate into monorepo

395a34e

The loopback4-example-getting-started repository is now being moved into the monorepo

kjdelisle force-pushed the getting-started branch from bbb3af5 to 395a34e Compare January 19, 2018 18:15

kjdelisle merged commit 9478d8b into master Jan 19, 2018

kjdelisle deleted the getting-started branch January 19, 2018 18:59

bajtos mentioned this pull request Jan 23, 2018

Add a deprecation message pointing to our monorepo strongloop-archive/loopback4-example-getting-started#19

Merged

		@@ -0,0 +1,25 @@
		Copyright (c) IBM Corp. 2017. All Rights Reserved.

		@@ -0,0 +1,25 @@
		Copyright (c) IBM Corp. 2017. All Rights Reserved.
		Node module: @loopback/getting-started

		@@ -0,0 +1,25 @@
		Copyright (c) IBM Corp. 2017-2018. All Rights Reserved.

		@@ -0,0 +1,14 @@
		// Copyright IBM Corp. 2017. All Rights Reserved.

		@@ -0,0 +1,14 @@
		// Copyright IBM Corp. 2017. All Rights Reserved.
		// Node module: loopback4-example-getting-started

		@@ -0,0 +1,57 @@
		import {Application, ApplicationConfig} from '@loopback/core';

		@@ -0,0 +1,12 @@
		import {DefaultCrudRepository, DataSourceType} from '@loopback/repository';

		@@ -0,0 +1,112 @@
		import {createClientForHandler, expect, supertest} from '@loopback/testlab';

feat(getting-started): migrate into monorepo #869

feat(getting-started): migrate into monorepo #869

Conversation

kjdelisle commented Jan 16, 2018 • edited by bajtos Loading

Checklist

kjdelisle commented Jan 16, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

virkt25 Jan 17, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Jan 18, 2018

bajtos 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

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjdelisle commented Jan 19, 2018

kjdelisle commented Jan 16, 2018 •

edited by bajtos

Loading

virkt25 Jan 17, 2018 •

edited

Loading