Implement getEndpoints by Amir-61 · Pull Request #305 · strongloop/strong-remoting

Amir-61 · 2016-05-25T21:23:25Z

This patch includes:

Implement getEndpoints which returns an array of objects, showing verbs and paths
Deprecate getHttpMethod
Deprecate getFullPath

bajtos · 2016-05-26T09:27:51Z

I am proposing to preserve backwards compatibility of this API to make migration easier, and provide new methods for the functionality we need. We can mark the old methods as deprecated, it will make it easier to find all places calling them.

RestMethod.prototype.getHttpMethod = function() {
  deprecated('getHttpMethod() is deprecated, use getFirstHttpMethod() instead.');
  return this.getFirstHttpMethod();
};

RestMethod.prototype.getFirstHttpMethod = function() {
  return this.getAllHttpMethods()[0];
};

RestMethod.prototype.getAllHttpMethods = function() {
  // the real works is done here
};

bajtos · 2016-05-26T09:28:48Z

      it('returns POST for `all`', function() {
        var method = givenRestStaticMethod({ http: { verb: 'all' }});
-        expect(method.getHttpMethod()).to.equal('POST');
+        expect(method.getHttpMethod()[0]).to.equal('POST');


With the ability to get all methods in place, we should test all members of the array returned.

expect(method.getAllHttpMethods()).to.eql(['POST']);

Amir-61 · 2016-05-31T23:24:00Z

Hey @rmg

I see lots of failures for dependencies like the following. Any thought about this? Thanks!

[sl-ci-run INFO  33.76 ]: npm install reports FAILURE

and

npm ERR! Error: connect ECONNREFUSED 10.0.1.45:32770
npm ERR!     at Object.exports._errnoException (util.js:893:11)
npm ERR!     at exports._exceptionWithHostPort (util.js:916:20)
npm ERR!     at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1075:14)
npm ERR!  { [Error: connect ECONNREFUSED 10.0.1.45:32770]
npm ERR!   code: 'ECONNREFUSED',
npm ERR!   errno: 'ECONNREFUSED',
npm ERR!   syscall: 'connect',
npm ERR!   address: '10.0.1.45',
npm ERR!   port: 32770,
npm ERR!   parent: 'debug' }
npm ERR! 
npm ERR! If you are behind a proxy, please make sure that the
npm ERR! 'proxy' config is set properly.  See: 'npm help config'

rmg · 2016-05-31T23:58:42Z

When that temporary registry is killed, anything trying to install from it will fail. This can happen if one of the dependents fails and triggers a registry shutdown.

Amir-61 · 2016-06-01T02:11:15Z

@bajtos PTAL :-)

This PR needs to be landed to unblock me from working on: strongloop/loopback#2316

bajtos · 2016-06-01T13:59:31Z

+  return this.getFirstHttpMethod();
+};
+
+RestMethod.prototype.getFirstHttpMethod = function() {


On the second thought, I think getFirstHttpMethod may be not worth the API pollution. Users can always call this.getEndpoints()[0].verb themselves. What do you think?

bajtos · 2016-06-01T14:01:31Z

@Amir-61 please add a test to verify that getEndpoints returns all paths.

Also what happens when you run the test suite, does it trigger deprecation warnings because we still call getMethod?

Amir-61 · 2016-06-01T19:42:01Z

@bajtos Please review another round :-)

Also what happens when you run the test suite, does it trigger deprecation warnings because we still call getMethod?

If you mean getHttpMethod and getFullPath, yes they show the deprecated messages. I personally think it is good to show deprecated message to let the users know what they were using
was retuning just the first route's verb/path. What is your thought?

Also, this generateRestMethodWithMultiplePaths may be messy. would you do it in the same way? How would you do that?

I'll squash commits into one single commit after your final LGTM

bajtos · 2016-06-02T11:17:13Z

If you mean getHttpMethod and getFullPath, yes they show the deprecated messages. I personally think it is good to show deprecated message to let the users know what they were using
was retuning just the first route's verb/path. What is your thought?

It's good to show deprecation messages for strong-remoting users, but it's not great to see them when running a test suite.

I am proposing the following:

Rework all tests using deprecated methods to use the new official methods
In unit-tests that are testing specifically these deprecated methods, temporarily disable deprecation messages via https://github.com/dougwilson/nodejs-depd#processenvno_deprecation

Also, this generateRestMethodWithMultiplePaths may be messy. would you do it in the same way? How would you do that?

I would simply call the existing helper givenRestStaticMethod instead of writing a new helper:

givenRestStaticMethod({ http: [
  { verb: 'DEL', path: '/testMethod1' },
  { verb: 'PUT', path: '/testMethod2' }
]});

Unless I am missing a reason why this would not work?

BTW I think we should backport this patch to 2.x too.

Amir-61 · 2016-06-03T03:22:18Z

+  before(function() {
+    process.env.NO_DEPRECATION = 'strong-remoting';
+  });
+


I am proposing the following:

Rework all tests using deprecated methods to use the new official methods

In unit-tests that are testing specifically these deprecated methods, temporarily disable deprecation messages via https://github.com/dougwilson/nodejs-depd#processenvno_deprecation

I reworked on where we were using deprecated getHttpMethod and getPath to use our new official methods, but here that we we are explicitly testing these deprecated methods, I should just have them untouched, and need to disable deprecation messages temporarily. I tried process.env.NO_DEPRECATION = 'strong-remoting'; but still it shows the deprecation messages...

Also it is worth mentioning that there are many other deprecated messages for other methods (not getHttpMethod or getPath)which are shown when you run tests for strong-remoting, I did not touch them because I thought it is out of scope of this PR.

Oh, I think I know what's the problem: depd initializes when the file is require-d, thus changing env.NO_DEPRECATION at runtime has no effect. Can you try to use process.on('deprecation') instead? See docs.

You also need an after handler to reset the environment (remove the listener), so that deprecations are printed again.

bajtos · 2016-06-03T14:24:37Z

 HttpInvocation.prototype.createRequest = function() {
  var method = this.method;
-  var verb = method.getHttpMethod();
+  var verb = method.getEndpoints()[0].verb;


perhaps store the endpoint, so that we don't have to repeat method.getEndpoints[0]?

var endpoint = method.getEndpoints()[0]); var verb = endpoints.verb; //... req.url = this.base + endpoint.fullPath;

Thoughts?

bajtos · 2016-06-03T15:06:16Z

@Amir-61 LGTM.

The current version disables deprecation warnings printed from all tests in test/rest-adapter.test.js, which may hide deprecations we actually want to be aware of. A better solution is to disable deprecation warnings only for those few tests where we are intentionally calling deprecated methods directly from the test. Let's do that in a new PR though, so that you can work on other things depending on the new API added by this PR too.

Also I think the new API should be back-ported to 2.x too, possibly without deprecation warnings. Thoughts?

Amir-61 · 2016-06-05T03:29:33Z

@bajtos I did not want to leave you unhappy so I did not land the PR to make sure you are happy with it :-) I fixed it now. please take a look again.

Also there are other deprecated methods (like sharedClass.find(), sharedClass.disableMethod(methodName, isStatic)) that we were testing specifically in shared-class.test.js and we were not disabling the deprecation messages for them either; however, since it was out of scope of this PR#305 I created an issue and I will take care of it soon in a separate PR; please see the ticket: #308

bajtos · 2016-06-06T13:01:45Z

+
+      after(function() {
+        process.removeListener('deprecation', NOOP);
+      });


Nitpick: add an empty line to visually delimit after/it blocks.

Idea to consider: extract these two before/after blocks into a shared helper method.

describe('getFullPath', function() { ignoreDeprecationsInThisBlock(); it('returns class path + method path', /*...*/); });

Thoughts?

I addressed this and pushed the change. Thanks!

bajtos · 2016-06-06T14:33:03Z

Lovely 👏

Could you please fix commit message summary to use getEndpoints instead of getEndPoints, in order to match the real API? Also the message body has typo "Deprectae" - should be "Deprecate".

Last but not least, please check why some of the dependants are failing, to make sure the failures are not related.

No further review is necessary.

* Implement getEndpoints * Deprecate getHttpMethod * Deprecate getFullPath

Amir-61 · 2016-06-06T15:55:59Z

@bajtos

Last but not least, please check why some of the dependants are failing, to make sure the failures are not related.

Unfortunately, failures are so random and they are not code relevant either. They all fail with this error: [sl-ci-run INFO 70.64 ]: npm install reports FAILURE. Please see this, this, this,... The same code on weekend passed about 47/50 tests, but now they are not passing; I guess the busier Jenkis is, the lower chance to pass tests :(

If I run many times by chance one of them may pass. I add the ci failures in our CI Failure report GH issue.

Considering the fact that failures are not code relevant, may I merge the code?

CC: @rmg

ADDED:

Oh nvm; after running tests a couple of more times. It eventually passed tests and now merging the code.

Amir-61 · 2016-06-06T15:58:16Z

@slnode test please

See strongloop/strong-remoting#305

Amir-61 added the #review label May 25, 2016

Amir-61 mentioned this pull request May 25, 2016

Expose Replace* methods strongloop/loopback#2316

Merged

bajtos reviewed May 26, 2016
View reviewed changes

Amir-61 self-assigned this May 27, 2016

Amir-61 changed the title ~~support fetching multiple http paths and methods~~ Implement getEndpoints May 31, 2016

Amir-61 force-pushed the ability_to_fetch_multiple_http_methods_paths branch from 7195b5c to c0c22ea Compare May 31, 2016 20:48

Amir-61 assigned bajtos Jun 1, 2016

Amir-61 assigned Amir-61 and unassigned Amir-61 Jun 1, 2016

bajtos reviewed Jun 1, 2016
View reviewed changes

bajtos removed their assignment Jun 1, 2016

Amir-61 assigned bajtos Jun 1, 2016

bajtos removed their assignment Jun 2, 2016

Amir-61 assigned bajtos Jun 2, 2016

Amir-61 reviewed Jun 3, 2016
View reviewed changes

Amir-61 force-pushed the ability_to_fetch_multiple_http_methods_paths branch from ca7b00f to b8acb0f Compare June 3, 2016 03:26

bajtos reviewed Jun 3, 2016
View reviewed changes

Amir-61 force-pushed the ability_to_fetch_multiple_http_methods_paths branch from 98dbeba to 18116de Compare June 5, 2016 01:51

bajtos reviewed Jun 6, 2016
View reviewed changes

Amir-61 force-pushed the ability_to_fetch_multiple_http_methods_paths branch from fed11a2 to edd0595 Compare June 6, 2016 13:23

bajtos assigned Amir-61 and unassigned bajtos and Amir-61 Jun 6, 2016

Implement getEndpoints

d44af88

* Implement getEndpoints * Deprecate getHttpMethod * Deprecate getFullPath

Amir-61 force-pushed the ability_to_fetch_multiple_http_methods_paths branch from edd0595 to d44af88 Compare June 6, 2016 15:40

Amir-61 merged commit b03dfe8 into master Jun 6, 2016

Amir-61 removed the #review label Jun 6, 2016

This was referenced Jun 6, 2016

Implement getEndpoints 2.x #309

Closed

Implement getEndpoints() [2.x] #310

Merged

Amir-61 deleted the ability_to_fetch_multiple_http_methods_paths branch June 6, 2016 18:14

Amir-61 mentioned this pull request Jun 7, 2016

Expose replace API endpoints strongloop/loopback#1979

Closed

davidcheung pushed a commit to strongloop/loopback-sdk-angular that referenced this pull request Jul 21, 2016

Update deprecated remoting metadata functions

29127db

See strongloop/strong-remoting#305

davidcheung mentioned this pull request Jul 21, 2016

Update deprecated remoting metadata functions strongloop/loopback-sdk-angular#229

Merged

davidcheung pushed a commit to strongloop/loopback-sdk-angular that referenced this pull request Jul 27, 2016

Update deprecated remoting metadata functions

97c2630

See strongloop/strong-remoting#305

Uh oh!

Conversation

Amir-61 commented May 25, 2016 • edited by bajtos Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bajtos commented May 26, 2016

Uh oh!

bajtos May 26, 2016

Choose a reason for hiding this comment

Uh oh!

Amir-61 commented May 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rmg commented May 31, 2016

Uh oh!

Amir-61 commented Jun 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bajtos Jun 1, 2016

Choose a reason for hiding this comment

Uh oh!

bajtos commented Jun 1, 2016

Uh oh!

Amir-61 commented Jun 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bajtos commented Jun 2, 2016

Uh oh!

Amir-61 Jun 3, 2016 • 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 Jun 3, 2016

Choose a reason for hiding this comment

Uh oh!

bajtos Jun 3, 2016

Choose a reason for hiding this comment

Uh oh!

bajtos Jun 3, 2016

Choose a reason for hiding this comment

Uh oh!

bajtos commented Jun 3, 2016

Uh oh!

Amir-61 commented Jun 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bajtos Jun 6, 2016

Choose a reason for hiding this comment

Uh oh!

Amir-61 Jun 6, 2016

Choose a reason for hiding this comment

Uh oh!

bajtos commented Jun 6, 2016

Uh oh!

Amir-61 commented Jun 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ADDED:

Uh oh!

Amir-61 commented Jun 6, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Amir-61 commented May 25, 2016 •

edited by bajtos

Loading

Amir-61 commented May 31, 2016 •

edited

Loading

Amir-61 commented Jun 1, 2016 •

edited

Loading

Amir-61 commented Jun 1, 2016 •

edited

Loading

Amir-61 Jun 3, 2016 •

edited

Loading

Amir-61 commented Jun 5, 2016 •

edited

Loading

Amir-61 commented Jun 6, 2016 •

edited

Loading