Skip to content

Conversation

@richardpringle
Copy link
Contributor

There is a new SharedClass method available in strong-remoting for disabling a method by name instead of specifying the isStatic boolean.

I have exposed the method on the Model Class such that it can be used for a spike related to #651.

@bajtos, please review

@richardpringle
Copy link
Contributor Author

@slnode test please

@richardpringle
Copy link
Contributor Author

@bajtos @0candy PTAL

@richardpringle
Copy link
Contributor Author

We can either land this or #2767

@bajtos
Copy link
Member

bajtos commented Sep 21, 2016

+1 for adding a new API that's consistent with strong-remoting API.

Could you please add a unit-test? At least copy & modify the existing test here that verifies remoteMethodDisabled event. It would be nice to have a test to verify that disabled methods are not accessible via REST too, but I am ok to land this PR even without that.

As for the existing Model.disableRemoteMethod method, I am proposing to

  • rework it to keep the same public API for backwards compatibility
  • call the new disableRemoteMethodByName under the hood
  • print a loopback-specific deprecation warning

See how SharedClass.prototype.disableMethod is implemented in strong-remoting (link) and use the same approach here.

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL on my comment above.

@bajtos bajtos added the #review label Sep 21, 2016
@richardpringle
Copy link
Contributor Author

@bajtos changes have been made as per your request

@bajtos
Copy link
Member

bajtos commented Sep 22, 2016

All three failed Windows builds are caused by Cannot start PhantomJS, I think it's ok to ignore this error.

@bajtos
Copy link
Member

bajtos commented Sep 22, 2016

The loopback-component-explorer failure is unrelated, see strongloop/loopback-component-explorer#178

@bajtos bajtos merged commit 2f0dd6d into master Sep 22, 2016
@bajtos bajtos deleted the fix/disableMethodByName branch September 22, 2016 07:39
@bajtos bajtos removed the #review label Sep 22, 2016
@bajtos
Copy link
Member

bajtos commented Sep 22, 2016

Landed, thank you @richardpringle. Could you please back-port the new API to 2.x (without the deprecation notice)?

@bajtos
Copy link
Member

bajtos commented Sep 22, 2016

@richardpringle we have a unit-test in loopback-component-explorer that's triggering a deprecation warning, could you PTAL?

strong-remoting deprecated SharedClass.prototype.disableMethod is deprecated. Use SharedClass.prototype.disableMethodByName instead. node_modules/loopback/lib/model.js:439:22
    ✓ updates swagger object when a remote method is disabled

Because the same version of loopback-component-explorer is supporting both LB 2.x and LB 3.x, it will be best to have the new API backported to 2.x, so that the tests in explorer can rely on this new API.

@richardpringle
Copy link
Contributor Author

richardpringle commented Sep 22, 2016

@bajtos:

Could you please back-port

backport -> #2781

LB-component-explorer PR -> strongloop/loopback-component-explorer#181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants