Allow defining remote method without isStatic flag #173

0candy · 2016-04-07T20:14:16Z

Connect to https://github.com/strongloop-internal/scrum-loopback/issues/813

Update generator-loopback remote-method to make isStatic prompt not required. If not specified, it will default to true if method name does not contain prototype. and false otherwise.
Add new validation for method name to allow for prototype. otherwise . is still invalid.
Add unit test cases.

bajtos · 2016-04-08T10:43:28Z

remote-method/index.js

-        default: true
+        default: function(answers) {
+            return !answers.methodName.match(/^prototype\.(.*)$/);
+          }


Indentation - L101 and L102 are indented by one extra level.

bajtos · 2016-04-08T10:47:09Z

@0candy could you please describe in pull request description what exactly are you changing here? I am mostly able to get that by reading the code, however other people and especially our users may not.

bajtos · 2016-04-08T10:51:01Z

As I commented in strongloop/loopback-workspace#273, I would like to preserve the current workspace API, i.e. a remote prototype method should still have the name without prototype. prefix and isStatic flag set to true.

bajtos · 2016-04-13T11:58:35Z

test/remote-method.test.js

+      var definition = common.readJsonSync('common/models/car.json');
+      var methods = definition.methods || {};
+      expect(methods).to.have.property('prototype.myRemote');
+      expect(methods['prototype.myRemote']).to.eql({


This is true when the project is using LoopBack 3.0, i.e. when common.createDummyProject creates a LB 3.0 project. However, since 3.0 was not released yet, I think we are using LB 2.0 and thus the entry should use the old convention (name + isStatic). Am I missing something?

bajtos · 2016-04-13T12:00:40Z

The implementation itself looks good to me, however the tests are suspicious. Let's figure out the loopback-workspace part fist and then come back to generator-loopback.

0candy · 2016-04-13T14:40:24Z

The change in generator-loopback was that either methods of defining the remote method should work. Only loopback-workspace distinguishes between LB 2.0 and 3.0.

bajtos · 2016-04-29T11:38:46Z

lib/helpers.js

+ * @returns {String|Boolean}
+ */
+exports.checkRemoteMethodName = function(name) {
+  if (name.match(/\./) && !name.match(/^prototype\.(.*)$/)) {


Is . character allowed after prototype.? I believe the current code allows that.

I think you need to change the second regexp to /^prototype\.([^.]*)$/.

Doesn't the \. after prototype include the . character?

Here is the test case to add to your unit-tests:

testValidationRejectsValue(validateRemoteMethodName, 'prototype.dotted.method');

bajtos · 2016-04-29T11:47:43Z

remote-method/index.js

        type: 'confirm',
-        default: true
+        default: function(answers) {
+          return !answers.methodName.match(/^prototype\.(.*)$/);


If we allow methodName to start with prototype. prefix, then I think we need to remove that prefix before we pass the name to ModelMethod constructor. See strongloop/loopback-workspace#273 (comment)

If we allow methodName to start with prototype. prefix, then I think we need to remove that prefix before we pass the name to ModelMethod constructor. See strongloop/loopback-workspace#273 (comment)

@0candy ^^^ thoughts?

I agree, hence I removed the prefix before passing the name to the constructor.

Ah, I see, the prefix is removed here: https://github.com/strongloop/generator-loopback/pull/173/files#diff-1162a393eb1e8979663fc397b9fbacbeR113

✅

bajtos · 2016-04-29T11:48:42Z

@0candy could you please add a unit-test to verify that the generator correctly handles the case where the user enters prototype.instanceMethod as the method name?

bajtos · 2016-05-03T14:29:55Z

lib/helpers.js

+ * @returns {String|Boolean}
+ */
+exports.validateRemoteMethodName = function(name) {
+  if (name.match(/\./) && !name.match(/^prototype\.(.*)$/)) {


Doesn't the \. after prototype include the . character?

Here is the test case to add to your unit-tests:

testValidationRejectsValue(validateRemoteMethodName, 'prototype.dotted.method');

ah, sorry. I misunderstood. Thought you said it doesn't allow prototype., but you meant any other . after prototype.. Fixed.

bajtos · 2016-05-04T14:27:08Z

Here is the test case to add to your unit-tests:
testValidationRejectsValue(validateRemoteMethodName, 'prototype.dotted.method');

@0candy I don't see that test case in the unit-tests. It makes me wonder, what is the reason behind that decision? We it perhaps left out by a mistake?

0candy · 2016-05-04T14:33:44Z

I did add it.
https://github.com/strongloop/generator-loopback/pull/173/files#diff-335b7c365ddfd4b8c9f944d8fba9f213R99

bajtos · 2016-05-04T14:36:09Z

lib/helpers.js

+ */
+exports.validateRemoteMethodName = function(name) {
+  if (name.match(/\./) && !name.match(/^prototype\.([^.]*)$/)) {
+    return 'Name cannot contain special characters . ' +


Nitpick: I think "special characters ." is not grammatically correct. How about the following?

'Name cannot contain "." characters except the dot in "prototype." prefix'

bajtos · 2016-05-04T14:36:41Z

One more nitpick, the patch LGTM otherwise. Please squash the commits before landing, no further review is needed.

bajtos · 2016-05-04T14:37:41Z

Oh, and please make sure CI builds are passing, they are all red ATM.

0candy · 2016-05-04T20:44:50Z

CI is all failing on master right now.

0candy · 2016-05-05T21:11:28Z

Yeoman's latest release has a breaking change in prompts. We probably have to update our code.
yeoman/generator#926

The latest version of yeoman-environment 1.6.1 is breaking the unit test. The promptModule is causing problems.

superkhau · 2016-05-11T22:48:57Z

@0candy Please let me know when you got this working. The fails are also making me cautious of merging #180 (comment)

0candy self-assigned this Apr 7, 2016

0candy added the #review label Apr 7, 2016

0candy force-pushed the update_remote_method branch from db48529 to b814032 Compare April 7, 2016 20:19

0candy changed the title ~~Include new style of defining remote method~~ Allow defining remote method without isStatic flag Apr 7, 2016

0candy force-pushed the update_remote_method branch 2 times, most recently from ed88078 to 31750fe Compare April 7, 2016 20:59

0candy assigned bajtos and unassigned 0candy Apr 7, 2016

bajtos reviewed Apr 8, 2016
View reviewed changes

bajtos assigned 0candy and unassigned bajtos Apr 8, 2016

0candy force-pushed the update_remote_method branch from 31750fe to fb1f66a Compare April 8, 2016 15:07

0candy assigned bajtos and unassigned 0candy Apr 13, 2016

bajtos reviewed Apr 13, 2016
View reviewed changes

0candy force-pushed the update_remote_method branch 2 times, most recently from 67806a5 to a9448c7 Compare April 25, 2016 18:22

bajtos reviewed Apr 29, 2016
View reviewed changes

bajtos assigned 0candy and unassigned bajtos Apr 29, 2016

bajtos reviewed Apr 29, 2016
View reviewed changes

0candy force-pushed the update_remote_method branch from a9448c7 to 15ec7ee Compare May 2, 2016 22:05

0candy assigned bajtos and unassigned 0candy May 2, 2016

bajtos reviewed May 3, 2016
View reviewed changes

bajtos assigned 0candy and unassigned bajtos May 3, 2016

0candy assigned bajtos and unassigned 0candy May 3, 2016

bajtos reviewed May 4, 2016
View reviewed changes

bajtos assigned 0candy and unassigned bajtos May 4, 2016

0candy force-pushed the update_remote_method branch 3 times, most recently from 49afc41 to 0ca7f26 Compare May 4, 2016 20:43

amuramoto mentioned this pull request May 11, 2016

Fixes datasource generation with community connector #180

Closed

Allow defining remote method without isStatic flag

6959163

0candy force-pushed the update_remote_method branch from 0ca7f26 to 6959163 Compare May 31, 2016 17:42

0candy merged commit 4a70425 into master May 31, 2016

0candy deleted the update_remote_method branch May 31, 2016 18:38

0candy removed the #review label May 31, 2016

Allow defining remote method without isStatic flag #173

Allow defining remote method without isStatic flag #173

Uh oh!

Conversation

0candy commented Apr 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Apr 8, 2016

Uh oh!

bajtos commented Apr 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Apr 13, 2016

Uh oh!

0candy commented Apr 13, 2016

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!

bajtos commented Apr 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented May 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0candy commented May 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented May 4, 2016

Uh oh!

bajtos commented May 4, 2016

Uh oh!

0candy commented May 4, 2016

Uh oh!

0candy commented May 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

superkhau commented May 11, 2016

Uh oh!

Uh oh!

bajtos commented May 4, 2016 •

edited

Loading

0candy commented May 5, 2016 •

edited

Loading