fix: updated combine url fix method

[Grabauskas]

Type: bug

Added more unit test cases and fixed combineBaseUrl method.
RegExp was changed to slice for optimization.

[codecov]

Codecov Report

Merging #1647 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1647      +/-   ##
==========================================
+ Coverage   84.86%   84.88%   +0.02%     
==========================================
  Files          47       47              
  Lines        2477     2481       +4     
  Branches      579      580       +1     
==========================================
+ Hits         2102     2106       +4     
  Misses        370      370              
  Partials        5        5

Impacted Files	Coverage Δ
src/lib/utils.ts	93.98% <100%> (+0.13%)	⬆️

[DanielRuf]

Suggested change

	if (prefix && prefix.length !== 0) {
	if (prefix && prefix.length) {

[Grabauskas]

Already tried that:

Prefer using an optional chain expression instead, as it's more concise and easier to read.eslint(@typescript-eslint/prefer-optional-chain)

[Grabauskas]

What's the point of prefix property type void?

Because that's why I can't use the optional chain operator:

if (prefix?.length) {

[juanpicado]

Why not _.isString()? We have used lodash all over the place anyway, also @DanielRuf says is not a critical code, it is called once in a time.

[DanielRuf]

Hi @Grabauskas,

thanks for your PR.

What exactly does this fix? Can you show a failing example or the relevant issue?

[DanielRuf]

Ah ok I see. One case that is fixed now.

Let me check if this can be simplified.
Performance is not critical here.

Summary of all failing tests
 FAIL  test/unit/modules/utils/utils.spec.ts
  ● Utilities › API utilities › combineBaseUrl › should create a base url for registry

    expect(received).toEqual(expected) // deep equality

    Expected: "http://domain"
    Received: ""

      214 |       test('should create a base url for registry', () => {
      215 |         expect(combineBaseUrl("http", 'domain', '')).toEqual('http://domain');
    > 216 |         expect(combineBaseUrl("http", 'domain', '/')).toEqual('http://domain');
          |                                                       ^
      217 |         expect(combineBaseUrl("http", 'domain', '/prefix/')).toEqual('http://domain/prefix');
      218 |         expect(combineBaseUrl("http", 'domain', '/prefix/deep')).toEqual('http://domain/prefix/deep');
      219 |         expect(combineBaseUrl("http", 'domain', 'only-prefix')).toEqual('only-prefix');

      at Object.toEqual (test/unit/modules/utils/utils.spec.ts:216:55)

[DanielRuf]

I would propose the following Return Early Pattern based approach:

export function combineBaseUrl(protocol: string, host: string | void, prefix?: string | void): string {
  let result = `${protocol}://${host}`;

  const prefixOnlySlash = prefix === '/';

  if (prefix && !prefixOnlySlash) {
    if (prefix.endsWith('/')) {
      prefix = prefix.slice(0, -1);
    }

    if (prefix.startsWith('/')) {
      return `${result}${prefix}`;
    }

    return prefix;
  }

  return result;
}

https://www.freecodecamp.org/forum/t/the-return-early-pattern-explained-with-javascript-examples/19364

Opinions @verdaccio/collaborators @verdaccio/core-team

[Grabauskas]

One more test can be added to check if triming is applied.

expect(combineBaseUrl("http", 'domain', ' ')).toEqual('http://domain');

[DanielRuf]

One more test can be added to check if triming is applied.

I don't know if this is a valid or common usecase.

[jamesgeorge007]

Suggested change

	result = prefix.length === 0 || prefix.indexOf('/') === 0 ? `${result}${prefix}` : prefix;
	result = prefix.length === 0 || prefix.startsWith('/') ? `${result}${prefix}` : prefix;

[DanielRuf]

All too complicated, also ternary operators are not great to read. See my proposed alternative solution.

#1647 (comment)

[juanpicado]

I would propose the following Return Early Pattern based approach:

export function combineBaseUrl(protocol: string, host: string | void, prefix?: string | void): string {
  let result = `${protocol}://${host}`;

  const prefixOnlySlash = prefix === '/';

  if (prefix && !prefixOnlySlash) {
    if (prefix.endsWith('/')) {
      prefix = prefix.slice(0, -1);
    }

    if(prefix.indexOf('/') === 0) {
      return `${result}${prefix}`;
    }

    return prefix;
  }

  return result;
}

https://www.freecodecamp.org/forum/t/the-return-early-pattern-explained-with-javascript-examples/19364

Opinions @verdaccio/collaborators @verdaccio/core-team

Much easier to read, I like it.

[juanpicado]

One more test can be added to check if triming is applied.

I don't know if this is a valid or common usecase.

We can trim in the one single place we use this function, I think the job of the method is combineBaseUrl rather clean up / formatting the input .

[DanielRuf]

We can trim in the one single place we use this function, I think the job of the is combineBaseUrl rather clean up / formatting the input .

Agreed 👍

[Grabauskas]

Updated by @DanielRuf solution.

[juanpicado]

expect(combineBaseUrl("http", 'domain', 'only-prefix')).toEqual('only-prefix');

🤔 ... this outcome is odd ... (not related with this PR but called my attention)

[Grabauskas]

In some cases url_prefix can be used as domain.
#622 (comment)

[juanpicado]

Thank u that makes totally sense .. then I would update the scenario with a real domain, a single string is a bit misleading.

[DanielRuf]

Will one of you create anew PR or issue for that?

[juanpicado]

Thanks @Grabauskas nice catch ! 👏

[lock]

🤖This thread has been automatically locked 🔒 since there has not been any recent activity after it was closed.
We lock tickets after 90 days with the idea to encourage you to open a ticket with new fresh data and to provide you better feedback 🤝and better visibility 👀.
If you consider, you can attach this ticket 📨 to the new one as a reference for better context.
Thanks for being a part of the Verdaccio community! 💘