Juggler array by shimks · Pull Request #1570 · loopbackio/loopback-next

shimks · 2018-07-27T21:13:05Z

The property definition for arrays used to be defined this way:

{type: 'string', array: true}
// or
{type: String, array: true}

This does not match with the property definition of an array as the legacy juggler would define it. The property definition created by the @property.array(String) decorator can now look like this:

{type: 'array', itemType: String}
// @repository-json-schema also supports
{type: 'array', itemType: 'string'}

Related to #1508
Fixes #1562

Checklist

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

bajtos · 2018-07-30T07:17:08Z

IIRC, it was @raymondfeng's intention to design the metadata used by @loopback/repository from scratch to build the best user experience and don't be constrained by the definition language used by juggler. Having said that, I am not sure if {type: 'string', array: true} was an intentional design - I would personally use {type: 'array', itemType: 'string'}.

Please note that it's the responsibility of legacy-juggler-bridge to convert model & property definitions from LB4 format to whatever is required by juggler. See DefaultCrudRepository#setupPersistedModel().

To be honest, I don't know how to proceed with this pull request. Ideally, I'd like to hear from @raymondfeng on the array property format. On the other hand, he is away until next week and I don't want to block fixing #1562 because of that.

Is there a way how to fix the issue while preserving the current format for defining array properties?

bajtos · 2018-07-30T07:10:31Z

packages/repository-json-schema/src/build-schema.ts

+export function stringTypeToWrapper(type: string | Function): Function {
+  if (typeof type === 'function') {
+    return type;
+  } else {


Please remove this else block to reduce indentation level and keep the changeset smaller.

if (typeof type === 'function') { return type; } type = type.toLowerCase(); let wrapper; switch(type) { // ...

bajtos · 2018-07-30T07:15:03Z

packages/repository-json-schema/test/unit/build-schema.unit.ts

 describe('build-schema', () => {
  describe('stringTypeToWrapper', () => {
-    it('returns respective wrapper of number, string and boolean', () => {
+    it('returns respective wrapper of number, string, boolean, array, buffer, date and object', () => {


One (logical) assert per test - please refactor this test into a series of small test cases.

it('returns String for "string"', () => { expect(stringTypeToWrapper('string')).to.eql(String); }); // ...

virkt25 · 2018-08-08T15:33:41Z

packages/repository-json-schema/test/integration/build-schema.integration.ts

-        }).to.throw(/type is defined as an array/);
-      });
-
-      it('errors out if "@property.array" is given "Array" as parameter', () => {


Why are all these tests removed?

These tests have been moved above for better organization, I'll quickly check if I actually removed any of the tests

EDIT: I see what you mean. The @property.array exception path has been removed so that, unless specified in the decorator, the metadata will just equate to {type: Array} without any information on what type array it is. It kind of is a decision I made on my own though. Should we still maintain the strictness of array types being specified (throwing an error when type is Array/'array' without itemType being set to something)?

virkt25 · 2018-08-08T15:35:07Z

packages/repository/src/repositories/legacy-juggler-bridge.ts

+    Object.entries(definition.properties).forEach(([key, value]) => {
+      if (value.type === 'array' || value.type === Array) {
+        value = Object.assign({}, value, {type: [value.itemType]});
+        delete value.itemType;


Do we need to delete value.itemType? This will get deleted from the actual definition.properties Object IIRC -- in case it needs to be referenced again.

Would it? I did want to avoid the manipulation of actual definition.properties by reassigning the variable value to what is a modified clone of value in line 120. Did I miss a step? This test case should cover it: https://github.com/strongloop/loopback-next/pull/1570/files#diff-328c8e879ea3a53d7fb50599f1c6a77eR121

Should I use a new variable for the modified cloned value to avoid the confusion?

nvm, the Object.assign creates a new reference. This is fine.

jannyHou · 2018-08-08T15:48:38Z

packages/cli/generators/model/index.js

+          val.tsType = 'Buffer[]';
        } else {
-          val.tsType = `string[]`;
+          val.tsType = 'string[]';


'string[]' ---> 'String[]' ?

I know string[] works but does String[] work as well?

sorry never mind :( I mixed it with the array type in juggler. string[] is correct.

found a String[] on line https://github.com/strongloop/loopback-next/pull/1570/files#diff-328c8e879ea3a53d7fb50599f1c6a77eR98...I feel confused again 🤔

I think they both work 🤷‍♂️

jannyHou · 2018-08-08T16:11:21Z

packages/repository-json-schema/src/build-schema.ts

+  let result: JSONSchema;
+  let propertyType = meta.type as string | Function;
+
+  if (isArrayType(propertyType) && meta.itemType) {


hmm...any reason why using two different ways to assert the array type?
isArrayType vs Array.isArray

isArrayType essentially asserts whether given data corresponds to the Wrapper Array or the string 'array'

jannyHou · 2018-08-08T17:12:46Z

I believe this PR would fix issue #1508 as well. The comment from @bajtos covered all my thought.

The signature {type: 'array', itemType: 'string'} looks good to me, so I would also like to know more about how we decided to take the other signature {type: 'string', array: true}.

virkt25

Clean up the commit history before merging but LGTM 🚢

…r the juggler

shimks requested review from bajtos and raymondfeng as code owners July 27, 2018 21:13

bajtos reviewed Jul 30, 2018

View reviewed changes

shimks mentioned this pull request Aug 2, 2018

fix(cli): change model template to properly render array types #1564

Merged

7 tasks

shimks force-pushed the juggler-array branch from 93e4275 to 0c62451 Compare August 7, 2018 15:41

bajtos mentioned this pull request Aug 8, 2018

Monthly Milestone - August 2018 🍉⛱ #1581

Closed

31 tasks

virkt25 reviewed Aug 8, 2018

View reviewed changes

jannyHou reviewed Aug 8, 2018

View reviewed changes

shimks force-pushed the juggler-array branch 2 times, most recently from a088aed to cad6c02 Compare August 8, 2018 17:13

raymondfeng approved these changes Aug 8, 2018

View reviewed changes

virkt25 reviewed Aug 9, 2018

View reviewed changes

virkt25 approved these changes Aug 9, 2018

View reviewed changes

fix(repository): change the way array property definition is built fo…

c6ebb6d

…r the juggler

shimks force-pushed the juggler-array branch from cad6c02 to c6ebb6d Compare August 10, 2018 15:50

shimks merged commit 2471c88 into master Aug 10, 2018

shimks deleted the juggler-array branch August 10, 2018 18:12

shimks mentioned this pull request Aug 10, 2018

Improperly Inserting Array Properties Into MongoDB #1508

Closed

Conversation

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

Uh oh!

Checklist

Uh oh!

bajtos commented Jul 30, 2018

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 Aug 8, 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!

Choose a reason for hiding this comment

Uh oh!

shimks Aug 8, 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!

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!

jannyHou commented Aug 8, 2018

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

shimks commented Jul 27, 2018 •

edited

Loading

shimks Aug 8, 2018 •

edited

Loading

shimks Aug 8, 2018 •

edited

Loading