Skip to content
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

feat: add method to get a base URL, allow to pass remote base #558

Merged
merged 2 commits into from
Jan 29, 2024

Conversation

Antreesy
Copy link
Contributor

@Antreesy Antreesy commented Jan 23, 2024

PR is adding:

  • getBaseUrl method to get the current instance;
  • UrlOptions.remoteBase to alter base URL in server request (if needed).

I'd say, there are most prominent use-cases which I see:

  • nextcloud-vue (to check for internal links);
  • Talk Federation (connecting to remote server with local credentials).

Tests:

getBaseUrl()
generateOcsUrl('apps/spreed/api/v4/room/{token}', { token: 'sfbhcekd' })
generateOcsUrl('apps/spreed/api/v4/room/{token}', { token: 'sfbhcekd' }, { remoteBase: 'https://remote.base.com' })

image

Copy link

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

New option is defined for

  • generateUrl
  • generateOcsUrl

But implementation only covers:

  • generateOcsUrl

@Antreesy
Copy link
Contributor Author

  • generateUrl

This method doesn't have a base part, so I assume, it's not applicable here?

export const generateUrl = (url: string, params?: object, options?: UrlOptions) => {
const allOptions = Object.assign({
noRewrite: false
}, options || {})
if (window?.OC?.config?.modRewriteWorking === true && !allOptions.noRewrite) {
return getRootUrl() + _generateUrlPath(url, params, options);
}
return getRootUrl() + '/index.php' + _generateUrlPath(url, params, options);
}

@nickvergessen
Copy link
Contributor

This method doesn't have a base part, so I assume, it's not applicable here?

But you need to overwrite getRootUrl() ?

What about generateRemoteUrl and linkToRemoteBase for attachment handling?

@Antreesy
Copy link
Contributor Author

Antreesy commented Jan 23, 2024

But you need to overwrite getRootUrl() ?

getBaseUrl already includes getRootUrl as suffix (at the end of string). But in places, where only root is used, we need to consider it somehow, you're right

What about generateRemoteUrl and linkToRemoteBase for attachment handling?

These methods are not affected by current changes.
We don't have yet a precedent, there it is needed, but if there is a necessity to connect to remote 'dav' service, then yes, I could modify it as well.

UPD: Covered now, I beleive

@Antreesy Antreesy force-pushed the feat/noid/get-base-url-method branch from e3c586b to 0585141 Compare January 23, 2024 14:45
@skjnldsv
Copy link
Contributor

remoteBase seems confusing to me.
It looks like we're talking about remote.php 😬

Do you have any other ideas for this variable name?

@nickvergessen
Copy link
Contributor

federation?

@skjnldsv
Copy link
Contributor

Or proxy something as a reverse proxy is a pretty standard concept.

@Antreesy
Copy link
Contributor Author

Antreesy commented Jan 23, 2024

I'd call it as it actually should be called, baseURL, like here, but this name is already used by Axios as one of the request options, and I'm afraid of interfering

@skjnldsv
Copy link
Contributor

baseURL seems to be the same as our use case?
Let's go for this?

@Antreesy
Copy link
Contributor Author

Let's go for this?

Tested, won't be a problem, unless you pass a relative URL to axios request

@Antreesy Antreesy force-pushed the feat/noid/get-base-url-method branch from 0585141 to 9640d02 Compare January 24, 2024 09:16
lib/index.ts Outdated Show resolved Hide resolved
lib/index.ts Outdated Show resolved Hide resolved
lib/index.ts Outdated Show resolved Hide resolved
@Antreesy Antreesy force-pushed the feat/noid/get-base-url-method branch from 9640d02 to 3457dcc Compare January 25, 2024 13:11
@susnux
Copy link
Contributor

susnux commented Jan 25, 2024

@Antreesy looks good to me, could you rebase and maybe add tests for the changed behavior?

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy Antreesy force-pushed the feat/noid/get-base-url-method branch from 3457dcc to 9826c00 Compare January 25, 2024 17:07
Copy link

codecov bot commented Jan 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (571c42f) 97.28% compared to head (bec5dd4) 97.54%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #558      +/-   ##
==========================================
+ Coverage   97.28%   97.54%   +0.25%     
==========================================
  Files           1        1              
  Lines         221      244      +23     
  Branches       33       36       +3     
==========================================
+ Hits          215      238      +23     
  Misses          5        5              
  Partials        1        1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…tance by default)

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy Antreesy merged commit 1a4ccc4 into master Jan 29, 2024
15 checks passed
@Antreesy Antreesy deleted the feat/noid/get-base-url-method branch January 29, 2024 11:21
@Antreesy Antreesy mentioned this pull request Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants