feat(rest): adds msgpack body parser by achrinza · Pull Request #6155 · loopbackio/loopback-next

achrinza · 2020-08-20T09:15:37Z

Signed-off-by: Rifa Achrinza 25147899+achrinza@users.noreply.github.com

Checklist

DCO (Developer Certificate of Origin) signed in all commits
npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

bajtos · 2020-08-20T13:01:56Z

Thank you @achrinza for the pull request! I'd like to discuss two aspects:

Is it a good idea to include msgpack support as a built-in parser? Can we ship it as an extension instead? I am concerned about adding msgpack dependency to all LB REST API users, even if they don't use msgpack.
Which library to use. In our KeyValue connectors, we use msgpack5 - see loopback-connector/lib/binary-package.js. msgpack5 has more downloads than msgpack and according to GitHub dependency graph, msgpack5 is also much more widely used than msgpack. There was no new version of msgpack5 published in 2 years, but then msgpack does not seem to be very actively maintained either (see Is this project dead? msgpack/msgpack-node#54).

bajtos · 2020-08-20T13:02:37Z

I like the idea of adding msgpack support to LoopBack ❤️ Just want to find a good solution 🤔

achrinza · 2020-08-20T13:08:15Z

Thanks for the feedback @bajtos.

I'm indifferent on whether to publish the msgpack body parser as a separate package. Though I'm inclined to agree that it can help reduce the bulk of @loopback/rest.
I haven't pushed the latest changes yet, but I'm experimenting towards using msgpack-lite instead. It has 3x more downloads than msgpack5, and the typings seems to be more complete.

Thoughts?

bajtos · 2020-08-20T14:29:22Z

I haven't pushed the latest changes yet, but I'm experimenting towards using msgpack-lite instead. It has 3x more downloads than msgpack5, and the typings seems to be more complete.

This package looks good too, although I am a bit worried that it's still in pre-1.0 version, the last version was published 4 years ago and their readme mentions Node.js 6 as the latest version being tested.

I wish there was a single well-maintained package for dealing with msgpack in Node.js :(

bajtos · 2020-08-20T14:31:49Z

Let's see if twitter-verse can offer any wisdom: https://twitter.com/bajtos/status/1296454814587588674

samarpanB · 2020-08-20T14:57:49Z

Thank you @achrinza for the pull request! I'd like to discuss two aspects:

Is it a good idea to include msgpack support as a built-in parser? Can we ship it as an extension instead? I am concerned about adding msgpack dependency to all LB REST API users, even if they don't use msgpack.

Which library to use. In our KeyValue connectors, we use msgpack5 - see loopback-connector/lib/binary-package.js. msgpack5 has more downloads than msgpack and according to GitHub dependency graph, msgpack5 is also much more widely used than msgpack. There was no new version of msgpack5 published in 2 years, but then msgpack does not seem to be very actively maintained either (see msgpack/msgpack-node#54).

I agree with @bajtos here and would recommend to keep this body parser as an extension. That would help to keep it separate and easier to maintain.

BTW, I like the idea of having this parser.

bajtos

Thank you @achrinza for reworking the patch into a standalone extension 👏🏻

Now I don't mind much which msgpack implementation we use, because changing to a different library is going to affect only msgpack users, majority of LB4 users won't even notice 👍🏻

I have few comments on the implementation details, PTAL below 👇🏻

extensions/bodyparser-msgpack/README.md

extensions/bodyparser-msgpack/package.json

extensions/bodyparser-msgpack/src/__tests__/unit/bodyparser.unit.ts

bajtos · 2020-08-21T07:53:40Z

Please check CI failures, for example https://travis-ci.com/github/strongloop/loopback-next/jobs/376022717#L298-L304:

> node bin/check-package-metadata.js

Some of the packages are not following loopback-next guidelines:

- @loopback/bodyparser-msgpack directory doesn't contain LICENSE file
- @loopback/bodyparser-msgpack is not added in the MONOREPO.md file
- @loopback/bodyparser-msgpack is not added in the CODEOWNERS.md file

(I think this is related to #6155 (comment)).

bodyparsers/msgpack/src/types.ts

achrinza · 2020-08-22T04:40:35Z

Travis Job (Click to expand)

Some of the packages are not following loopback-next guidelines:

- @loopback/test-extension-logging-fluentd is not added in the MONOREPO.md file

- @loopback/test-repository-cloudant is not added in the MONOREPO.md file

- @loopback/test-repository-mongodb is not added in the MONOREPO.md file

- @loopback/test-repository-mysql is not added in the MONOREPO.md file

- @loopback/test-repository-postgresql is not added in the MONOREPO.md file

- @loopback/tsdocs-monorepo is not added in the MONOREPO.md file

- @loopback/tsdocs is not added in the MONOREPO.md file

- @loopback/sandbox-example is not added in the MONOREPO.md file

A bit stumped why ./bin/update-monorepo-file.js doesn't include the all the projects; Tried running npm ci beforehand in hopes of it solving the issue to no avail.

Any suggestions?

Edit: Fixed by #6175

bajtos

The new version looks mostly good, I have few more observations to discuss.

The CI build is still failing, PTAL.

bodyparsers/msgpack/README.md

bodyparsers/msgpack/package.json

bodyparsers/msgpack/src/parserlibrary.ts

packages/express/src/middleware-registry.ts

bodyparsers/msgpack/package.json

bodyparsers/msgpack/src/component.ts

bajtos

You are making great progress!

bodyparsers/msgpack/README.md

bajtos · 2020-09-01T14:52:42Z

bodyparsers/msgpack/README.md

+    //
+    this._res
+      .type('application/msgpack')
+      .end(msgpack.encode(this.todoRepository.find(filter)));


Can you please update import statements in this example to show where is msgpack imported from?

I think it would be best if our users can use the same version of msgpack that's used for parsing the requests, i.e.

import {msgpack} from '@loopback/bodyparser-msgpack';

Also in the light of the discussion in https://github.com/strongloop/loopback-next/pull/6155/files#r481194436, maybe we should rename the extension to @loopback/rest-msgpack? (In the future, we may want to add msgpack support for other transports like messaging, so the rest- prefix should make it clear this extension is for REST.)

maybe we should rename the extension to @loopback/rest-msgpack

Just an idea to consider, maybe it's too early considering that this package will provide just the body parser for quite some time until we implement support for content-type negotiation in the framework.

I think it makes sense to rename it to @loopback/rest-msgpack.

bodyparsers/msgpack/dist/__tests__/acceptance/bodyparser.acceptance.d.ts

bajtos · 2020-09-11T08:09:58Z

feat(rest): adds msgpack body parser

nitpick: we use imperative tense in commit messages, i.e. add msgpack body parser

bajtos

Almost there! I have few minor comments to consider, PTAL below.

Please wait few more days before landing, to get @raymondfeng a chance to review the final version too.

bodyparsers/rest-msgpack/README.md

bajtos · 2020-09-11T08:15:46Z

docs/site/Using-MessagePack-with-LoopBack.md

+layout: readme
+source: loopback-next
+file: bodyparsers/msgpack/README.md
+permalink: /doc/en/lb4/Using-messagepack-with-loopback.html


AFAICT, this page is not referenced from the sidebar, can we remove it?

As of loopbackio/loopback.io#996, we allow sidebar entries (group headings) with no linked page.

It's being referenced here: https://github.com/strongloop/loopback-next/pull/6155/files#diff-967acfe8350a9043a788970b5cf91223R236

bajtos · 2020-09-11T08:16:18Z

docs/site/sidebars/lb4_sidebar.yml

+    - title: 'Using MessagePack'
+      output: 'web, pdf'
+      children:
+      - title: 'How to use MessagePack with LoopBack'


Are you expecting to have multiple pages under Using MessagePack section? To me, this feels like an extra nesting.

Good point, I've removed the nesting.

It was initially taken from the TypeORM extension

Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>

achrinza added community-contribution REST Issues related to @loopback/rest package and REST transport in general labels Aug 20, 2020

achrinza force-pushed the feat/adds-msgpack-bodyparser branch 3 times, most recently from eb11c4c to ac3db6c Compare August 20, 2020 09:37

achrinza self-assigned this Aug 20, 2020

achrinza force-pushed the feat/adds-msgpack-bodyparser branch 3 times, most recently from 4f1862d to ed552f6 Compare August 20, 2020 10:19

achrinza force-pushed the feat/adds-msgpack-bodyparser branch from 5c47ac8 to 0e9f000 Compare August 21, 2020 06:44

bajtos suggested changes Aug 21, 2020

View reviewed changes

raymondfeng reviewed Aug 22, 2020

View reviewed changes

bodyparsers/msgpack/src/types.ts Outdated Show resolved Hide resolved

achrinza mentioned this pull request Aug 22, 2020

fix: monorepo, package metadata scripts' filepath on Windows #6175

Merged

2 tasks

bajtos reviewed Aug 27, 2020

View reviewed changes

raymondfeng reviewed Aug 28, 2020

View reviewed changes

bodyparsers/msgpack/src/component.ts Outdated Show resolved Hide resolved

achrinza force-pushed the feat/adds-msgpack-bodyparser branch 2 times, most recently from 124ca6d to 8b7145e Compare August 29, 2020 09:47

bajtos reviewed Sep 1, 2020

View reviewed changes

achrinza force-pushed the feat/adds-msgpack-bodyparser branch 4 times, most recently from 1c1be37 to 6faf732 Compare September 4, 2020 23:57

achrinza marked this pull request as ready for review September 5, 2020 00:15

achrinza requested a review from bajtos September 5, 2020 00:15

bajtos approved these changes Sep 11, 2020

View reviewed changes

achrinza force-pushed the feat/adds-msgpack-bodyparser branch from 6faf732 to 0503592 Compare September 12, 2020 07:48

feat(rest): add msgpack body parser

9906078

Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>

achrinza force-pushed the feat/adds-msgpack-bodyparser branch from 0503592 to 9906078 Compare September 12, 2020 08:22

achrinza requested a review from raymondfeng September 16, 2020 08:28

raymondfeng approved these changes Sep 17, 2020

View reviewed changes

raymondfeng merged commit 4e5a128 into loopbackio:master Sep 17, 2020

achrinza deleted the feat/adds-msgpack-bodyparser branch September 17, 2020 23:32

Conversation

achrinza commented Aug 20, 2020

Checklist

Uh oh!

bajtos commented Aug 20, 2020

Uh oh!

bajtos commented Aug 20, 2020

Uh oh!

achrinza commented Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bajtos commented Aug 20, 2020

Uh oh!

bajtos commented Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samarpanB commented Aug 20, 2020

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bajtos commented Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

achrinza commented Aug 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bajtos Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

bajtos Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

bajtos Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

achrinza Sep 4, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bajtos commented Sep 11, 2020

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bajtos Sep 11, 2020

Choose a reason for hiding this comment

Uh oh!

achrinza Sep 12, 2020 • 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!

bajtos Sep 11, 2020

Choose a reason for hiding this comment

Uh oh!

achrinza Sep 12, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

achrinza commented Aug 20, 2020 •

edited

Loading

bajtos commented Aug 20, 2020 •

edited

Loading

bajtos commented Aug 21, 2020 •

edited

Loading

achrinza commented Aug 22, 2020 •

edited

Loading

achrinza Sep 12, 2020 •

edited

Loading