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

Remove spurious AppSubUrl in serviceworker request. #16047

Merged
merged 5 commits into from
Jun 8, 2021

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Jun 1, 2021

There is another spurious AppSubUrl placement in the serviceworker registration.
This PR removes it.

Signed-off-by: Andrew Thornton art27@cantab.net

There is another spurious AppSubUrl placement in the serviceworker registration.
This PR removes it.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath added type/bug issue/regression Issue needs no code to be fixed, only a description on how to fix it yourself skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Jun 1, 2021
@zeripath zeripath added this to the 1.15.0 milestone Jun 1, 2021
@zeripath zeripath requested a review from silverwind June 1, 2021 19:05
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jun 1, 2021
Signed-off-by: Andrew Thornton <art27@cantab.net>
@@ -44,7 +44,7 @@ export default async function initServiceWorker() {
// normally we'd serve the service worker as a static asset from AssetUrlPrefix but
// the spec strictly requires it to be same-origin so it has to be AppSubUrl to work
Copy link
Member

Choose a reason for hiding this comment

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

This comment no longer matches the code, it needs to be updated

@silverwind
Copy link
Member

Are you sure it's removable? Does it still work with the app on a subdirectory?

@silverwind
Copy link
Member

I wonder why we actually put it on AppSubUrl in first place. The purpose of the SW is to manage assets below AssetUrlPrefix so I think the correct URL might just be ${AssetUrlPrefix}/serviceworker.js which should also work in multi-domain deployments with assets on a separate domain.

@zeripath
Copy link
Contributor Author

zeripath commented Jun 2, 2021

I run gitea on a subdirectory as my testing platform so that's why I spot these. Removing this makes this correct for me.

I don't run with a static URL prefix so I don't know what those URLs should look like - but I doubt that they should have AppSuburl either.

It may be helpful to add comments to the static URL prefix go "constant" similar to those I added to AppURL and AppSuburl which state whether or not they have preceding or suffixed '/'s - this has saved me multiple times as I every time I use one (at least in go) vscode tells me whether I need to add a / or not.

@silverwind
Copy link
Member

silverwind commented Jun 2, 2021

Regarding slashes, we should just ensure the backend variables never contain a trailing slash (e.g. stripping it from config var), that way we can remove the joinPath function in JS too.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 8, 2021
@zeripath zeripath merged commit e03a91a into go-gitea:main Jun 8, 2021
@zeripath zeripath deleted the fix-serviceworker-url branch June 8, 2021 17:46
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 10, 2021
There is another spurious AppSubUrl placement in the serviceworker registration.
This PR removes it.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@go-gitea go-gitea locked and limited conversation to collaborators Oct 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/regression Issue needs no code to be fixed, only a description on how to fix it yourself lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants