Skip to content

Remove spurious AppSubUrl in serviceworker request.#16047

Merged
zeripath merged 5 commits into
go-gitea:mainfrom
zeripath:fix-serviceworker-url
Jun 8, 2021
Merged

Remove spurious AppSubUrl in serviceworker request.#16047
zeripath merged 5 commits into
go-gitea:mainfrom
zeripath:fix-serviceworker-url

Conversation

@zeripath
Copy link
Copy Markdown
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 Indicates a previously functioning feature or behavior that has broken or regressed after a change 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
zeripath added 2 commits June 1, 2021 21:31
Signed-off-by: Andrew Thornton <art27@cantab.net>
Comment thread web_src/js/features/serviceworker.js Outdated
@@ -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
Copy Markdown
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
Copy Markdown
Member

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

@silverwind
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

zeripath added 2 commits June 2, 2021 10:33
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 Indicates a previously functioning feature or behavior that has broken or regressed after a change 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