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

Allow url_prefix to be only the path #197

Merged
merged 4 commits into from May 21, 2017

Conversation

Projects
None yet
3 participants
@BartDubois
Contributor

BartDubois commented May 18, 2017

Type: feature

The following has been addressed in the PR:

  • There is a related issue

Description:

  • Define utils function get_base_url.
  • If url_prefix start with / construct base URL using protocol and host form request.
  • Update SERVER.md with description of new url_prefix option.

Resolves #193

Allow url_prefix to be only the path
 * Define utils function `get_base_url`.
 * If url_prefix start with `/` construct base URL using protocol and host form request.
 * Update SERVER.md with description of new `url_prefix` option.
@Meeeeow

This comment has been minimized.

Member

Meeeeow commented May 20, 2017

jointURL / joint_url may better than get_base_url? @juanpicado

* @return {String} base registry url
*/
function get_base_url(protocol, host, prefix) {
let result = protocol + '://' + host;

This comment has been minimized.

@juanpicado

juanpicado May 20, 2017

Member

Please template strings here. always ES6 whenever is possible

This comment has been minimized.

@BartDubois

BartDubois May 21, 2017

Contributor

Done

* @param {String} prefix
* @return {String} base registry url
*/
function get_base_url(protocol, host, prefix) {

This comment has been minimized.

@juanpicado

juanpicado May 20, 2017

Member

As @Meeeeow suggested, this name seems get the url from somewhere. Either joinUrl or better naming you might propose. Also rename as camelCase, is not something we enforce with eslint but I'll change that soon.

This comment has been minimized.

@BartDubois

BartDubois May 21, 2017

Contributor

Changed to combineBaseUrl.
I was considering combineBaseUrlFrom or baseUrlFrom then example usage could looks like:

let base = Utils.baseUrlFrom(req.protocol, req.get('host'), config.url_prefix);

I use such conventions from time to time (strict separation command from query) and found that it makes code more readable, but require more attention. So finally I keep it without From suffix.

@juanpicado juanpicado added this to the 2.1.8 milestone May 20, 2017

BartDubois added some commits May 21, 2017

Aplly sugestions form review
* Reneame the methof and use camelCase
* Use ES6 template string
Fix ES6 template string
should be ${...} not @{...}
if (prefix) {
prefix = prefix.replace(/\/$/, '');
result = (prefix.indexOf('/') === 0)
? result + prefix
? '@{result}@{prefix}'

This comment has been minimized.

@juanpicado
function get_base_url(protocol, host, prefix) {
let result = protocol + '://' + host;
function combineBaseUrl(protocol, host, prefix) {
let result = '@{protocol}://@{host}';

This comment has been minimized.

@juanpicado

juanpicado May 21, 2017

Member

Same here, template literals, not single quotes.

* @param {String} prefix
* @return {String} base registry url
*/
function combineBaseUrl(protocol, host, prefix) {

This comment has been minimized.

@juanpicado

juanpicado May 21, 2017

Member

👍 I like the name :)

This comment has been minimized.

@BartDubois

BartDubois May 21, 2017

Contributor

:)

Fix ES string delimiters
Use ` in stand of '
@codecov

This comment has been minimized.

codecov bot commented May 21, 2017

Codecov Report

❗️ No coverage uploaded for pull request base (master@93bea50). Click here to learn what that means.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #197   +/-   ##
=========================================
  Coverage          ?   79.14%           
=========================================
  Files             ?       23           
  Lines             ?     2210           
  Branches          ?        0           
=========================================
  Hits              ?     1749           
  Misses            ?      461           
  Partials          ?        0
Impacted Files Coverage Δ
lib/index-web.js 56.04% <100%> (ø)
lib/utils.js 89.13% <75%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93bea50...5336a0a. Read the comment docs.

@juanpicado

This comment has been minimized.

Member

juanpicado commented May 21, 2017

Cool, if Travis-ci is happy, all we are happy, thanks @BartDubois . Ignore codecov (still under testing)

@juanpicado juanpicado merged commit 16f8ae0 into verdaccio:master May 21, 2017

2 of 4 checks passed

codecov/patch No report found to compare against
Details
codecov/project No report found to compare against
Details
codacy/pr Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@BartDubois

This comment has been minimized.

Contributor

BartDubois commented May 21, 2017

Thank you.

I'm sorry for the the "mess" with ES6 strings. I do not used it yet in my day to day work... I will improve this element next time :).

Best

@BartDubois BartDubois deleted the BartDubois:url_prefix-as-path branch May 21, 2017

@juanpicado

This comment has been minimized.

Member

juanpicado commented May 21, 2017

No worries :) I "mess" code at work too 😅😅. Thanks again.

@juanpicado juanpicado added bug and removed need verification labels Aug 26, 2017

@lock

This comment has been minimized.

lock bot commented May 31, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators May 31, 2018

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