Bring in loopback4-example-log-extension #908

bajtos · 2018-01-25T13:47:12Z

The first commit 76558a6 brings in an exact copy of https://github.com/strongloop/loopback4-example-log-extension.

The second commit f4dd44e contains changes necessary to integrate the example with the rest of the monorepo.

See #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

shimks · 2018-01-25T15:32:31Z

I've noticed that this repo doesn't use MetadataInspector from @loopback/metadata. Should we migrate this first and then look to refactor the repo?

virkt25

Looks great! Just have some minor nits.

virkt25 · 2018-01-25T15:15:07Z

packages/example-getting-started/package.json

    "build:dist6": "lb-tsc es2015",
    "build:apidocs": "lb-apidocs",
-    "clean": "lb-clean loopback-getting-started*.tgz dist dist6 package api-docs",
+    "clean": "lb-clean *example-getting-started*.tgz dist dist6 package api-docs",


Is the * before the word example necessary?

Yes. Here is the full tarball name as produced by npm pack:

loopback-example-getting-started-1.0.1-alpha.0.tgz

virkt25 · 2018-01-25T15:21:28Z

packages/example-log-extension/LICENSE

@@ -0,0 +1,21 @@
+MIT License
+
+Copyright (c) 2017 StrongLoop and IBM API Connect


I believe all the other packages use a slightly different header for LICENSE. I think this should use the same.

Copyright (c) IBM Corp. 2017,2018. All Rights Reserved. Node module: @loopback/example-log-extension This project is licensed under the MIT License, full text below. -------- MIT license

Good catch, I'll use LICENSE from getting-started example.

virkt25 · 2018-01-25T15:24:39Z

packages/example-log-extension/README.md

@@ -0,0 +1,364 @@
+# loopback4-example-log-extension


Should we change this to example-log-extension?

I'll use @loopback/example-log-extension for consistency with getting-started example (I'll fix package.json too).

virkt25 · 2018-01-25T15:26:18Z

packages/example-log-extension/index.d.ts

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


2017, 2018.

(All files -- since others in loopback-next were already updated).

Let's use slt license because that's the tool we will use in the future.

SLT_OWNER="IBM Corp." slt copyright

Unfortunately this tool sets the copyright year to 2018 because it uses git commit history to find out the date of the first commit. Do you think this is a problem?

virkt25 · 2018-01-25T15:26:41Z

packages/example-log-extension/index.d.ts

@@ -0,0 +1,6 @@
+// Copyright IBM Corp. 2017. All Rights Reserved.
+// Node module: loopback4-example-log-extension


Node module: @loopback/example-log-extension.

virkt25 · 2018-01-25T15:28:06Z

packages/example-log-extension/package.json

@@ -0,0 +1,55 @@
+{
+  "name": "loopback4-example-log-extension",


@loopback/example-log-extension.

virkt25 · 2018-01-25T15:28:37Z

packages/example-log-extension/package.json

@@ -0,0 +1,55 @@
+{
+  "name": "loopback4-example-log-extension",
+  "version": "1.0.0",


example getting started is version as 4.0.0-alpha.0. Should we use the same here?

virkt25 · 2018-01-25T15:29:48Z

packages/example-log-extension/package.json

+  },
+  "repository": {
+    "type": "git",
+    "url": "git+https://github.com/strongloop/loopback4-example-log-extension.git"


Point to loopback-next repo.

virkt25 · 2018-01-25T15:35:05Z

packages/example-log-extension/test/mocha.opts

@@ -0,0 +1,2 @@
+--recursive


This can be dropped since package.json uses mocha.opts from @loopback/build.

virkt25 · 2018-01-25T15:35:38Z

packages/example-log-extension/test/smoke.test.ts

+// This file is licensed under the MIT License.
+// License text available at https://opensource.org/licenses/MIT
+
+import {expect} from '@loopback/testlab';


I think it's safe to drop this file as well.

shimks

Left few comments about some things to think about, ~~but aside from that LGTM~~ on top of what @virkt25 has already mentioned

shimks · 2018-01-25T15:37:09Z

packages/example-getting-started/package.json

    "build:current": "lb-tsc",
    "build:dist": "lb-tsc es2017",
    "build:dist6": "lb-tsc es2015",
    "build:apidocs": "lb-apidocs",


Should we be creating apidocs for this repo?

shimks · 2018-01-25T15:38:39Z

packages/example-log-extension/LICENSE

@@ -0,0 +1,21 @@
+MIT License
+
+Copyright (c) 2017 StrongLoop and IBM API Connect


virkt25 · 2018-01-25T15:40:07Z

@shimks I agree. It's probably best to create a issue to follow up on the refactor but this PR's scope should be migration only.

bajtos · 2018-01-26T12:27:42Z

@virkt25 @shimks I think I have addressed all your review comments in ~~e80ddc1~~ 324e81a, LGTY now?

virkt25 · 2018-01-26T15:01:29Z

packages/rest/index.js


 const nodeMajorVersion = +process.versions.node.split('.')[0];
-module.exports = nodeMajorVersion >= 7 ? require('./dist') : require('./dist6');
+module.exports = nodeMajorVersion >= 7 ?


Please revert the changes to this file.

virkt25

Please revert changes to packages/rest/index.js.

LGTM. No further review needed from me.

Copy the contents of loopback4-example-log-extension, leave out dot files like gitignore

bajtos added this to the Sprint 54 milestone Jan 25, 2018

bajtos self-assigned this Jan 25, 2018

bajtos requested review from kjdelisle and virkt25 January 25, 2018 13:47

bajtos requested a review from raymondfeng as a code owner January 25, 2018 13:47

bajtos added the needs-priority label Jan 25, 2018

virkt25 mentioned this pull request Jan 25, 2018

Bring examples back into the monorepo #836

Closed

12 tasks

bajtos removed the needs-priority label Jan 25, 2018

bajtos force-pushed the example/log-extension branch from 8e1c215 to f4dd44e Compare January 25, 2018 14:03

virkt25 reviewed Jan 25, 2018

View reviewed changes

shimks approved these changes Jan 25, 2018

View reviewed changes

bajtos force-pushed the example/log-extension branch from d255e8c to e80ddc1 Compare January 26, 2018 12:26

bajtos requested a review from virkt25 January 26, 2018 12:27

b-admike approved these changes Jan 26, 2018

View reviewed changes

bajtos force-pushed the example/log-extension branch from e80ddc1 to 324e81a Compare January 26, 2018 13:43

virkt25 reviewed Jan 26, 2018

View reviewed changes

virkt25 approved these changes Jan 26, 2018

View reviewed changes

docs: add example-log-extension

fec2122

Copy the contents of loopback4-example-log-extension, leave out dot files like gitignore

bajtos force-pushed the example/log-extension branch from 324e81a to fec2122 Compare January 26, 2018 16:21

bajtos merged commit b9883ec into master Jan 26, 2018

bajtos deleted the example/log-extension branch January 26, 2018 16:59

bajtos mentioned this pull request Jan 26, 2018

fix: make mocha self-contained with the source map support #913

Merged

6 tasks

		@@ -0,0 +1,21 @@
		MIT License

		Copyright (c) 2017 StrongLoop and IBM API Connect

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

		@@ -0,0 +1,6 @@
		// Copyright IBM Corp. 2017. All Rights Reserved.
		// Node module: loopback4-example-log-extension

Bring in loopback4-example-log-extension #908

Bring in loopback4-example-log-extension #908

Uh oh!

Conversation

bajtos commented Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

shimks commented Jan 25, 2018

Uh oh!

virkt25 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shimks left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shimks Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

virkt25 commented Jan 25, 2018

Uh oh!

bajtos commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

virkt25 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bajtos commented Jan 25, 2018 •

edited

Loading

shimks left a comment •

edited

Loading

shimks Jan 25, 2018 •

edited

Loading

bajtos commented Jan 26, 2018 •

edited

Loading