Add remoting options to server/config.json #164

raymondfeng · 2014-10-21T17:56:59Z

See strongloop/loopback#608
See strongloop/loopback#337

bajtos · 2014-10-22T06:36:37Z

templates/components/api-server/template/server/boot/rest-api.js

-  server.use(restApiRoot, server.loopback.rest());
+  var remotingOptions = server.get('remoting') || {};
+  var restOptions = remotingOptions.rest || {};
+  server.use(restApiRoot, server.loopback.rest(restOptions));


The general approach is to keep the templates as small as possible and implement most features in loopack. That way we can roll bugfixes without having to ask the users to upgrade the generated code.

AFAIK, loopback.rest has access to the app object it is mounted on, thus it can read app.get('remoting') itself, there is no need to pass it down from here.

How to access the app instance from loopback.rest? I understand the app instance is available from req.app.

See lib/middleware/rest.js#L27, it is already accessing the app.

The trick is that you have the application available only at request-handling time, not at the time you are building the middleware/handler function.

bajtos · 2014-10-22T06:42:05Z

templates/components/api-server/component.js

-    { name: 'port', value: 3000 }
+    { name: 'port', value: 3000 },
+    { name: 'remoting', value: {
+      rest: {context: {enableHttpContext: false}, normalizeHttpPath: false},


The comments in strongloop/loopback#608 indicate that normalizeHttpPath is read from remoting.normalizeHttpPath, not remoting.rest.normalizeHttpPath.

Please write an automated test to verify we got this right. Something along the lines:

Assert that remoting.context.normalizeHttpPath is defined as false in the scaffolded app. That way the test fails if we change the default config in the future (e.g. move the key to a different place).

Edit server/config.json and set normalizeHttpPath to true.

Setup anything else needed to verify that http paths are normalized.

Start the server, verify that http paths are normalized now.

Ideally, there should be one test for each of the config options. To be realistic, I think it should be ok to test only enableHttpContext and normalizeHttpPath.

Also the default config is missing enableContext option as mentioned in strongloop/loopback#337

bajtos · 2014-10-22T06:57:09Z

I feel the proposed config option structure is not optimal.

{
  remoting: {
    rest: {context: {enableHttpContext: false}, normalizeHttpPath: false},
    json: {strict: false, limit: '100kb'},
    urlencoded: {extended: true, limit: '100kb'},
    cors: {origin: true, credentials: true}
  }
}

Assuming the most common usage is to enable context middleware either for all transports (REST, JSON-RPC, WebSocket) or none of them, it makes more sense to have the context options on the top level.

{
  remoting: {
    context: { enable: true, someOtherContextOptionSharedByAllTransports: 'value' },
    rest: { enableHttpContext: false, normalizeHttpPath: false},
    json: {strict: false, limit: '100kb'},
    urlencoded: {extended: true, limit: '100kb'},
    cors: {origin: true, credentials: true}
  }
}

I am not entirely sure where does enableHttpContext belongs to, whether it should be remoting.context.enableHttpContext, remoting.rest.enableHttpContext or remoting.rest.context.enableHttpContext as originally proposed.

The goal of this exercise is to approach this problem from the POV of the future user and come up with a solution that will be easiest to understand and use, instead of using the current implementation details to drive the user interface.

raymondfeng · 2014-10-22T17:58:41Z

Does normalizeHttpPath apply to REST only?

bajtos · 2014-10-22T18:27:18Z

Does normalizeHttpPath apply to REST only?

Yes, AFAIK. I assume JSON-RPC and WebSocket uses method names (e.g. User.create).

bajtos · 2014-10-30T11:26:34Z

See also strongloop/loopback#708.

raymondfeng · 2014-10-31T23:00:01Z

@bajtos I updated the remoting options to be:

{
      context: {enableHttpContext: false},
      rest: {normalizeHttpPath: false},
      json: {strict: false, limit: '100kb'},
      urlencoded: {extended: true, limit: '100kb'},
      cors: {origin: true, credentials: true}
}

The changes to boot/rest-api.js is also reverted.

bajtos · 2014-11-05T19:06:47Z

Is it worth adding errorHandler section too? See https://github.com/strongloop/strong-remoting/pull/114/files and strongloop/loopback#688.

bajtos · 2014-11-05T19:07:22Z

templates/components/api-server/component.js

+      rest: {normalizeHttpPath: false},
+      json: {strict: false, limit: '100kb'},
+      urlencoded: {extended: true, limit: '100kb'},
+      cors: {origin: true, credentials: true}


Please fix the coding style - add space inside curly brackets.

raymondfeng · 2014-11-30T23:09:01Z

Fixed the style and added errorHandler.

bajtos · 2014-12-01T10:14:41Z

templates/components/api-server/component.js

+          enableHttpContext: false
+        },
+        rest: {
+          normalizeHttpPath: false


Do you want to add xml: false too?

See strongloop/strong-remoting#134

bajtos · 2014-12-01T10:16:53Z

It would be nice to add rest.xml option too, but it can be done in a new PR too.

I did not verify that the values in template match LoopBack's defaults.

LGTM.

raymondfeng · 2014-12-01T16:35:54Z

Added rest.xml and rest.supportedTypes.

Add remoting options to server/config.json

bajtos · 2014-12-01T18:19:06Z

templates/components/api-server/component.js

+        rest: {
+          normalizeHttpPath: false,
+          xml: false,
+          supportedTypes: ['json', 'application/javascript', 'text/javascript']


This is not great. It means that to enable xml, you have to edit both xml and supportedTypes now.

Before this PR, it was enough to add xml:true and developers did not need to worry about content types.

Should I remove supportedTypes: ['json', 'application/javascript', 'text/javascript']?

Yes, that will be the best.

Here are some alternatives:

Change the value of supportedTypes to list all types including xml types. I find that a bit confusing, plus it is difficult to upgrade. You have to upgrade your config file if you want to use a new JSON-based type like application/vnd.api+json.

Keep the current value, tell people to delete supportedTypes together with changing xml:true. However, the problem with upgrades is still present.

See b682a70

👍 thanks

raymondfeng added the #review label Oct 21, 2014

bajtos reviewed Oct 22, 2014
View reviewed changes

bajtos self-assigned this Oct 22, 2014

bajtos reviewed Oct 22, 2014
View reviewed changes

bajtos mentioned this pull request Oct 22, 2014

Add context propagation middleware strongloop/loopback#337

Merged

bajtos assigned raymondfeng and unassigned bajtos Oct 22, 2014

bajtos mentioned this pull request Oct 30, 2014

rest handler options strongloop/loopback#708

Merged

raymondfeng force-pushed the feature/add-remoting-options branch from 169b34d to 6abf5b1 Compare October 31, 2014 22:58

bajtos reviewed Nov 5, 2014
View reviewed changes

raymondfeng force-pushed the feature/add-remoting-options branch from 6abf5b1 to 09494ad Compare November 30, 2014 23:08

bajtos reviewed Dec 1, 2014
View reviewed changes

Add remoting options to server/config.json

d0a87c8

raymondfeng force-pushed the feature/add-remoting-options branch from 09494ad to d0a87c8 Compare December 1, 2014 16:35

raymondfeng added a commit that referenced this pull request Dec 1, 2014

Merge pull request #164 from strongloop/feature/add-remoting-options

81bee6c

Add remoting options to server/config.json

raymondfeng merged commit 81bee6c into master Dec 1, 2014

raymondfeng removed the #review label Dec 1, 2014

raymondfeng deleted the feature/add-remoting-options branch December 1, 2014 17:29

bajtos reviewed Dec 1, 2014
View reviewed changes

bajtos mentioned this pull request Jan 23, 2017

Configure remoting options via app.set and config.json strongloop/loopback#608

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add remoting options to server/config.json #164

Add remoting options to server/config.json #164

raymondfeng commented Oct 21, 2014

bajtos Oct 22, 2014

raymondfeng Oct 22, 2014

bajtos Oct 22, 2014

bajtos Oct 22, 2014

bajtos Oct 22, 2014

bajtos Oct 22, 2014

bajtos commented Oct 22, 2014

raymondfeng commented Oct 22, 2014

bajtos commented Oct 22, 2014

bajtos commented Oct 30, 2014

raymondfeng commented Oct 31, 2014

bajtos commented Nov 5, 2014

bajtos Nov 5, 2014

raymondfeng commented Nov 30, 2014

bajtos Dec 1, 2014

bajtos Dec 1, 2014

bajtos commented Dec 1, 2014

raymondfeng commented Dec 1, 2014

bajtos Dec 1, 2014

raymondfeng Dec 1, 2014

bajtos Dec 1, 2014

raymondfeng Dec 1, 2014

bajtos Dec 1, 2014

Add remoting options to server/config.json #164

Add remoting options to server/config.json #164

Conversation

raymondfeng commented Oct 21, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Oct 22, 2014

raymondfeng commented Oct 22, 2014

bajtos commented Oct 22, 2014

bajtos commented Oct 30, 2014

raymondfeng commented Oct 31, 2014

bajtos commented Nov 5, 2014

Choose a reason for hiding this comment

raymondfeng commented Nov 30, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Dec 1, 2014

raymondfeng commented Dec 1, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment