Skip to content

Support normalizeHttpPath - #433

Closed
fabien wants to merge 2 commits into
strongloop:masterfrom
fabien:feature/normalize-http-path
Closed

Support normalizeHttpPath#433
fabien wants to merge 2 commits into
strongloop:masterfrom
fabien:feature/normalize-http-path

Conversation

@fabien

@fabien fabien commented Aug 4, 2014

Copy link
Copy Markdown
Contributor

This options should probably become the default at some point (opt-out
instead of opt-in).

Thoughts on setting this globally (config.json) are welcome.

Mandatory PR: strongloop/strong-remoting#90

@bajtos

bajtos commented Sep 18, 2014

Copy link
Copy Markdown
Member

The patch LGTM, we should probably wait with the merge until a new version of strong-remoting is released.

@fabien can you please rebase on the latest master and also sign the CLA using the new cla-hub based tool? Sorry for the hassle.

@bajtos bajtos self-assigned this Sep 24, 2014
@bajtos

bajtos commented Sep 24, 2014

Copy link
Copy Markdown
Member

@fabien ping, do you have time to finish this PR?

@fabien

fabien commented Sep 24, 2014

Copy link
Copy Markdown
Contributor Author

I will when I return on monday. Please remind me if I forget.

@fabien ping, do you have time to finish this PR?


Reply to this email directly or view it on GitHub.

Comment thread lib/application.js

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A nice to have feature: allow developers to turn on/off the flag normalizeHttpPath via server/config.json.

options = extend(this.get('remoting') || {}, options);
var remotes = this.remotes();
var handler = this._handlers[type] = remotes.handler(type, options);

Usage in config.json:

{
  "port": 3000,
  "restApiRoot": "/api",
  "remoting": {
    "normalizeHttpPath": true
  }
}

@ritch @raymondfeng thoughts?
@fabien it's ok to leave this feature out of scope of this PR.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nice to have feature: allow developers to turn on/off the flag normalizeHttpPath via server/config.json.

good idea 👍

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's move the implementation outside of this PR - see #608.

@bajtos

bajtos commented Oct 2, 2014

Copy link
Copy Markdown
Member

FWIW, the patch LGTM.

@bajtos

bajtos commented Oct 3, 2014

Copy link
Copy Markdown
Member

@fabien can you please rebase on the latest master and also sign the CLA using the new cla-hub based tool? Sorry for the hassle.

When it's done, I'd like Jenkins to verify all tests passes and then the PR can be landed.

@bajtos

bajtos commented Oct 7, 2014

Copy link
Copy Markdown
Member

@fabien ping

@bajtos bajtos assigned fabien and unassigned bajtos Oct 8, 2014
@fabien
fabien force-pushed the feature/normalize-http-path branch from fa3123a to cf5c153 Compare October 11, 2014 14:50
@ritch ritch added the blocked label Oct 15, 2014
@raymondfeng

Copy link
Copy Markdown
Member

@bajtos ping

Comment thread test/access-token.test.js

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FWIW, I believe this line was removed from the current master.

@bajtos

bajtos commented Oct 22, 2014

Copy link
Copy Markdown
Member

I have rebased the patch on the current master, squashed commits, fixed coding style issues (mostly trailing spaces), added a descriptive commit message and landed as 568c866

@bajtos bajtos removed the blocked label Oct 23, 2014
@bajtos

bajtos commented Oct 23, 2014

Copy link
Copy Markdown
Member

This patch did not change loopback.rest to pass options to app.handler - see #688.

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.

4 participants