Skip to content
This repository was archived by the owner on Jan 16, 2022. It is now read-only.

fix: correctly reference registry url from options#300

Merged
priscilawebdev merged 8 commits intoverdaccio:masterfrom
pmmmwh:patch-1
Nov 24, 2019
Merged

fix: correctly reference registry url from options#300
priscilawebdev merged 8 commits intoverdaccio:masterfrom
pmmmwh:patch-1

Conversation

@pmmmwh
Copy link
Copy Markdown
Contributor

@pmmmwh pmmmwh commented Nov 21, 2019

Type: Bug Fix

Description:

Currently, the repository URL is not being correctly referenced in the Registry Info dialog if the web UI is not loaded at root. This is discovered and illustrated here. This PR uses window.__VERDACCIO_UI_OPTIONS to correctly get the base url so that loading will not affect the display of info.

@juanpicado
Copy link
Copy Markdown
Member

Thank u so much for this !

anikethsaha
anikethsaha previously approved these changes Nov 23, 2019
Copy link
Copy Markdown
Member

@anikethsaha anikethsaha left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@juanpicado
Copy link
Copy Markdown
Member

Only test are failing

@anikethsaha
Copy link
Copy Markdown
Member

I think tests need to be updated.

image

registry URL should not contain the path, right?

  • http://localhost/-/web/detail is not a registry URL.
  • http://localhost it is

cc @juanpicado , what do you think ?

@priscilawebdev
Copy link
Copy Markdown
Contributor

thanks for your contribution @pmmmwh :)

Unfortunately tests are failing..

@pmmmwh
Copy link
Copy Markdown
Contributor Author

pmmmwh commented Nov 23, 2019

thanks for your contribution @pmmmwh :)

Unfortunately tests are failing..

I'll check and update accordingly - I think it is mostly because of window.__VERDACCIO_BASENAME_UI_OPTIONS not containing the correct information.

Edit: I fixed most of the stuff. Should I remove the tests related to gegetRegistryURL too? (since we now do not have logic on manipulating the trailing slash) Or - should I update the test so that it can test when the base url is different it will get the correct one?

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 24, 2019

Codecov Report

Merging #300 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master    #300   +/-   ##
======================================
  Coverage    88.2%   88.2%           
======================================
  Files         143     143           
  Lines         975     975           
  Branches      194     193    -1     
======================================
  Hits          860     860           
  Misses         97      97           
  Partials       18      18
Impacted Files Coverage Δ
src/utils/url.ts 20% <100%> (ø) ⬆️

@ant1m4tt3r
Copy link
Copy Markdown
Contributor

Looks good to me

@priscilawebdev
Copy link
Copy Markdown
Contributor

priscilawebdev commented Nov 24, 2019

Now it looks good! Thank you @pmmmwh 👏🙃

@priscilawebdev priscilawebdev merged commit ee74474 into verdaccio:master Nov 24, 2019
@juanpicado
Copy link
Copy Markdown
Member

juanpicado commented Nov 24, 2019

Sorry guys, but, the info dialog is broken. https://github.com/verdaccio/ui/issues/310

Screen Shot 2019-11-24 at 7 52 52 PM

I'd suggest a new PR with :

export function getRegistryURL(): string {
  return window.__VERDACCIO_BASENAME_UI_OPTIONS.base;
}

We don't use server rendering, thus, the base only include that, the base domain, but does not include the path or possible queries. As consequence the window.__VERDACCIO_BASENAME_UI_OPTIONS.base will return always / as you can see in the screenshot above.

@priscilawebdev
Copy link
Copy Markdown
Contributor

priscilawebdev commented Nov 24, 2019

Hi @juanpicado that is true..I have just checked and it's broken :( I reversed the changes . Thank you.

@juanpicado
Copy link
Copy Markdown
Member

It's fine, I think the problem is bigger that is seems so, it might need some changes in the backend. I introduced this bug with this verdaccio/verdaccio#1457

@pmmmwh
Copy link
Copy Markdown
Contributor Author

pmmmwh commented Nov 25, 2019

Sorry guys, but, the info dialog is broken. #310

Screen Shot 2019-11-24 at 7 52 52 PM

I'd suggest a new PR with :

export function getRegistryURL(): string {
  return window.__VERDACCIO_BASENAME_UI_OPTIONS.base;
}

We don't use server rendering, thus, the base only include that, the base domain, but does not include the path or possible queries. As consequence the window.__VERDACCIO_BASENAME_UI_OPTIONS.base will return always / as you can see in the screenshot above.

Oops - I'm sorry about that. I'll see if there's another way to achieve the correct behaviour.

juanpicado pushed a commit that referenced this pull request Jan 9, 2020
* Revert "Revert "fix(#300): correctly reference registry url from options" (#311)"

This reverts commit d955268.

* fix: generate full URL from path
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants