search

strongloop / strong-remoting

Communicate between objects in servers, mobile apps, and other servers.
www.strongloop.com
Other
105 stars 93 forks source link

Implement getEndpoints #305

Closed Amir-61 closed 8 years ago

Amir-61 commented 8 years ago

This patch includes:

/to: @bajtos

bajtos commented 8 years ago

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
};
Amir-61 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@bajtos PTAL :-)

This PR needs to be landed to unblock me from working on: https://github.com/strongloop/loopback/pull/2316

bajtos commented 8 years ago

@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 commented 8 years ago

@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?

bajtos commented 8 years ago

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:

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.

bajtos commented 8 years ago

@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 commented 8 years ago

@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: https://github.com/strongloop/strong-remoting/issues/308

bajtos commented 8 years ago

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.

Amir-61 commented 8 years ago

@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 commented 8 years ago

@slnode test please