Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rest handler options #708

Merged
merged 1 commit into from
Oct 31, 2014
Merged

rest handler options #708

merged 1 commit into from
Oct 31, 2014

Conversation

gcirne
Copy link
Contributor

@gcirne gcirne commented Oct 29, 2014

Adds support for options to rest handler. See strongloop/strong-remoting#118 for some background.

/cc @bajtos
/cc @raymondfeng

@bajtos
Copy link
Member

bajtos commented Oct 30, 2014

Nice.

  1. Please add a jsdoc comment for the newly added function arg.
  2. Write a unit-test verifying that the options are passed down the line. You can use disableStackTrace: true to verify that the adapter behaviour was changed.
  3. Options should default to (app.get('remoting') || {}).rest - see my comment elsewhere. If you prefer to leave this part out of this PR then I am fine with that too.

@bajtos
Copy link
Member

bajtos commented Oct 30, 2014

@slnode ok to test

@bajtos
Copy link
Member

bajtos commented Oct 30, 2014

@slnode test please

@gcirne
Copy link
Contributor Author

gcirne commented Oct 30, 2014

Sure, no problem. I'll get to this as soon as possible.
On qui, 30 de out de 2014 at 09:30 Miroslav Bajtoš notifications@github.com
wrote:

@slnode https://github.com/slnode test please


Reply to this email directly or view it on GitHub
#708 (comment).

@gcirne
Copy link
Contributor Author

gcirne commented Oct 30, 2014

Hey @bajtos,

I added the missing jsdoc.

About the missing test. I added it as a pending test. I cannot get it to pass unless I change https://github.com/strongloop/strong-remoting/blob/master/lib/rest-adapter.js#L280 to read root.use(RestAdapter.errorHandler(this.remotes.options.errorHandler || this.options.errorHandler));

Any pointers?

@bajtos
Copy link
Member

bajtos commented Oct 30, 2014

Interesting, I did not realise the options were sourced from remotes.

IMO options passed trough remotes.handler('rest', options) should override remotes.options, i.e. you need to swap the items in the expression.

Other than that, I am ok with changing L280 as part of this patch.

@bajtos
Copy link
Member

bajtos commented Oct 30, 2014

ok, not as part of this patch, since it is in a different project, but as part of getting this patch landed.

Alternatively is there any other option you can use in the test to verify the options object was passed down the line?

@gcirne
Copy link
Contributor Author

gcirne commented Oct 30, 2014

Not that I can think of. The other option that I know of is the recently landed supportedTypes. But that hasn't been released yet so we sort of have the chicken and the egg problem.

What do you think about releasing a new version of strong-remoting so that I can test against that?

@bajtos
Copy link
Member

bajtos commented Oct 30, 2014

I'd rather wait with a strong-remoting release until Raymond's changes are landed.

However, you can install strong-remoting from github master for the purpose of writing and running the tests. Our CI build uses the latest master revision too, so that the tests will pass there too.

That way you can continue the work without having to wait for a strong-remoting release.

@gcirne
Copy link
Contributor Author

gcirne commented Oct 30, 2014

Ok, I'll do that then.

@gcirne
Copy link
Contributor Author

gcirne commented Oct 30, 2014

Added test and squashed commits against current master.

@gcirne
Copy link
Contributor Author

gcirne commented Oct 30, 2014

@bajtos apparently the test is failing because it did not run against strong-remoting's current master. Anything I need to do?

@raymondfeng
Copy link
Member

I just published strong-remoting 2.8.0.

@slnode test please

@gcirne
Copy link
Contributor Author

gcirne commented Oct 31, 2014

@bajtos or @raymondfeng do you mind clicking the Restart Build button in Travis? I don't have permission!

https://travis-ci.org/strongloop/loopback/builds/39525645

@bajtos
Copy link
Member

bajtos commented Oct 31, 2014

We have two CI servers for strongloop/loopback: a Travis CI and an internal Jenkins-based CI. Travis does not pick strong-remoting master, Jenkins does.

@bajtos
Copy link
Member

bajtos commented Oct 31, 2014

@slnode test please

The Travis build is green: https://travis-ci.org/strongloop/loopback/builds/39525645

bajtos added a commit that referenced this pull request Oct 31, 2014
@bajtos bajtos merged commit b527405 into strongloop:master Oct 31, 2014
@bajtos
Copy link
Member

bajtos commented Oct 31, 2014

I am not sure why @slnode is not willing to build this PR. /cc @rmg

The Travis is green, I have landed the patch. Thank you @gcirne for the contribution 👍

@bajtos
Copy link
Member

bajtos commented Oct 31, 2014

I am afraid I misunderstood how the remoting options are implemented and gave you a wrong advice on how to implement this whole feature :(

At the moment, all other remoting options are read from remotes.options, which are set from app.get('remoting') the first time app.remotes() is called (typically when a first model is attached to the app).

I don't want to introduce a new inconsistent way of configuring the remoting adapters, thus I have to revert this patch.

@gcirne gcirne deleted the rest-options branch October 31, 2014 15:45
@gcirne
Copy link
Contributor Author

gcirne commented Oct 31, 2014

Ok.

@masdevid
Copy link

masdevid commented Nov 3, 2014

Hello, I'm new using loopback. I don't know where else to ask but how to force API response in json format?

@bajtos
Copy link
Member

bajtos commented Nov 4, 2014

@masdevid edit your server/config.json file and add a "remoting" section like this one:

{
  "remoting": {
    "rest": {
      "supportedTypes": ["json", "application/javascript", "text/javascript"]
    }
  }
}

@bajtos
Copy link
Member

bajtos commented Nov 4, 2014

Don't forget to update to the latest strong-remoting version, e.g. via rm -rf node_modules && npm install.

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.

6 participants