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

Trailing slash resolves in 404 #23

Closed
preetamslot opened this issue Sep 16, 2023 · 31 comments · Fixed by #27
Closed

Trailing slash resolves in 404 #23

preetamslot opened this issue Sep 16, 2023 · 31 comments · Fixed by #27

Comments

@preetamslot
Copy link

Hi,
The pwa works great but I would like to know how to redirect
/blog/post/ to blog/post trailing slashes in the url are not resolving.

It is a static build

build: {
    format: "file"
  },
AstroPWA({
      mode: "production",
      base: "/",
      scope: "/",
      includeAssets: [`icons/${process.env.THEME}/favicon.svg`],
      registerType: "autoUpdate",
      manifest: manifest,
      workbox: {
        navigateFallback: "/404",
        globPatterns: ["**/*.{js,css,html,txt,json,svg,woff,woff2,ico,xml}"],
      },
      devOptions: {
        enabled: false,
        navigateFallbackAllowlist: [/^\/404$/],
      },
    }),
@felixroos
Copy link

I have a similar problem, where trailing slashes always end up on the root path

@userquin
Copy link
Member

Can you configure tralingSlash in Astro? https://github.com/vite-pwa/astro/blob/main/src/index.ts#L54

@felixroos
Copy link

Can you configure tralingSlash in Astro? https://github.com/vite-pwa/astro/blob/main/src/index.ts#L54

thanks for the quick answer. yes I've now done that, trying both combinations.

  • trailingSlash: 'always' + build.format: 'directory': only trailing slashes work, without trailing slashes the service worker loads the root page. A hard refresh gives a 404
  • trailingSlash: 'never' + build.format: 'file': same as above but flipped.. this is how I have currently deployed it.

Example Links:

The optimal case would be if it did not matter if a trailing slash is used or not.
I am not entirely sure if this is a vite / astro / vite-pwa or github pages problem...

@felixroos
Copy link

Just found: https://github.com/slorber/trailing-slash-guide which explains the behavior of github pages for different links, notably:

/file -
/file/ - 💢 404
/folder - ➡️ /folder/
/folder/ -

This further confirms that build.format: 'file' will cause a 404 with a trailing slash.
Without using a service worker, it looks like it would work with build.format: 'directory', as it will just do a redirect.
So is suspect that the service worker will interpret urls without trailing slashes as a file and for some reason serve the root file.

@felixroos
Copy link

I have done a quick test repro with a simple astro project that builds using build.format: 'directory': https://github.com/felixroos/astro-test/

Both of these urls work:

So it seems the problem is really service worker related..

@userquin
Copy link
Member

userquin commented Oct 20, 2023

IIRC there is a directoryIndex and cleanUrl in workbox option:
https://developer.chrome.com/docs/workbox/modules/workbox-precaching/#directory-index

EDIT: maybe both options wth another names, the link is about using the workbox modules.

@userquin
Copy link
Member

I Will try to check your repro tmr.

@felixroos
Copy link

felixroos commented Oct 20, 2023

IIRC there is a directoryIndex and cleanUrl in workbox option: https://developer.chrome.com/docs/workbox/modules/workbox-precaching/#directory-index

EDIT: maybe both options wth another names, the link is about using the workbox modules.

On first sight that sounds like it would help in this case!

I Will try to check your repro tmr.

Thanks!

For completeness sake, I've now done a full reproduction that shows how adding @vite-pwa/astro to the mix results in wrong caching behavior:

@userquin
Copy link
Member

@felixroos

can you deploy using mode: 'development' in pwa options (https://github.com/vite-pwa/astro/blob/main/examples/pwa-simple/astro.config.mjs#L14)? this way you will see the sw logs, I'll change the logic in the pwa integration to allow configure external manifestTransform, rn the pwa integration will add always a new one to handle Astro directory and trailing slash options.

@userquin
Copy link
Member

Since you're deploying to gh pages, the scope is the project name without final slash (check the sw pre cache manifest in your deployed app) and so the sw maybe intercepting the /test/, I'll fix that also (GH Pages redirecting to /astro-test-pwa/ when requesting /astro-test-pwa

@userquin
Copy link
Member

userquin commented Oct 21, 2023

Preparing PR to not modify the sw precache manifest when using directory, on local works for /test/, /test, /test/about and /test/about/, check the pwa-simple example (added same folders).

@userquin
Copy link
Member

Astro build is inconsistent, why 404/index.html missing? only 404.html in root dist folder

@userquin
Copy link
Member

userquin commented Oct 21, 2023

@felixroos can you test in local with current PR or review it (GH Pages maybe will fail from your url since /test/ will return 404)? There is nothing we can do here to solve it, you should use trailingSlash !== 'always' and /test/ will always fail (once the sw installed it should work since it will use directoryIndex using test/index.html in the cache, if you request it on first request it should fail).

@userquin
Copy link
Member

userquin commented Oct 21, 2023

You can open the pwa-example using this StackBlitz repro.

EDIT: stop dev server and run npm run build && npm run preview, I have no idea why open in new window stops working in StackBlitz (we cannot open dev tools to see workbox messages; on my local Open in new Window doesn't work and so cannot see workbox logs)

@felixroos
Copy link

can you deploy using mode: 'development' in pwa options? this way you will see the sw logs

deployed, but it only logs "SW registered: ..." atm

@felixroos can you test in local with current PR or review it

can test tomorrow (if it's still relevant)

(GH Pages maybe will fail from your url since /test/ will return 404)?

with build.format: 'directory', /test/ should work, as it will lead to /test/index.html which exists.
it also works rn: https://felixroos.github.io/astro-test-pwa/test/ (with hard refresh)

the stackblitz example seems to work fine!

@userquin
Copy link
Member

userquin commented Oct 22, 2023

@felixroos don't forget to include registerType: 'autoUpdate' in your pwa options, the new sw will be awaiting to confirm update (default register type is prompt).

It seems SB has fixed the Open in new window link.

I'll check why mode is not working, maybe Astro build changing it, on local workbox messages are there:
imagen

@felixroos
Copy link

https://felixroos.github.io/astro-test-pwa/ now deployed with autoUpdate + menu

@userquin
Copy link
Member

userquin commented Oct 22, 2023

@felixroos can you try building the branch and using file protocol to install it in your repo?

  • clone this repo (all branches)
  • checkout userquin/fix-allow-custom-manifest-transform branch
  • pnpm install && pnpm build && pnpm pack
  • copy tgz file and paste it in your repo (add it to git)
  • in your repo: pnpm remove -D @vite-pwa/astro && pnpm add -D ./vite-pwa-astro-0.1.3.tgz
  • in your repo pnpm build && pnpm preview and check it is working
  • deploy to GH Pages

@felixroos
Copy link

done: https://felixroos.github.io/astro-test-pwa/ works!

@userquin
Copy link
Member

userquin commented Oct 22, 2023

@felixroos Now the dilemma in this repo with the PR, I'm afraid to put it as is, perhaps with a flag to activate it?

EDIT: in any case, the owner can plug its own manifestTransform and do whatever (s)he wants

@felixroos
Copy link

@felixroos Now the dilemma in this repo with the PR, I'm afraid to put it as is, perhaps with a flag to activate it?

EDIT: in any case, the owner can plug its own manifestTransform and do whatever (s)he wants

You mean because an opt-in flag won't break existing setups that rely on the old behavior?

@userquin
Copy link
Member

userquin commented Oct 22, 2023

You mean because an opt-in flag won't break existing setups that rely on the old behavior?

YES (I'll check it next week, maybe you can update the PR, I need to rest...)

@felixroos
Copy link

I'm not sure as I have too little knowledge of this project to give my opinion on it. Of course, having less config options is always a plus, but maybe the old behavior is sometimes desired, not sure. From my perspective this looks more like a bug that should probably be fixed by default, but maybe it introduces some regression.
Maybe start with the feature flag and test it with a handful of existing projects to see if it has downsides?
Either way, I would be very happy to be able to use this as it will finally fix this annoying bug I was having for some time now.

@felixroos
Copy link

You mean because an opt-in flag won't break existing setups that rely on the old behavior?

YES (I'll check it next week, maybe you can update the PR, I need to rest...)

I can see what I can do, please give yourself some rest. I can also wait or use the tgz

@userquin
Copy link
Member

The problem, I don't use Astro, I switched to Nuxt 3... This repo should be in Astro repo, but the PR was closed a few times without any explanation and finally added here (íles integration for example in the repo not in Vite Org)

@userquin
Copy link
Member

@felixroos I'll add it under experimental, like PWA VitePress Integration, check this PR: vite-pwa/vitepress#23

@felixroos
Copy link

@felixroos I'll add it under experimental, like PWA VitePress Integration, check this PR: vite-pwa/vitepress#23

thanks!

@userquin
Copy link
Member

@felixroos can you run the same process with the branch in this PR in you GH repro and deploy to GH Pages?

  • clone this repo (all branches)
  • checkout userquin/feat-experimental-trailing-slashes-handler branch
  • pnpm install && pnpm build && pnpm pack
  • in your repo: run pnpm remove -D @vite-pwa/astro then remove vite-pwa-astro-0.1.3.tgz file
  • copy new tgz file and paste it in your repo (add it again to git)
  • in your repo: pnpm add -D ./vite-pwa-astro-0.1.3.tgz
  • include in pwa options:
experimental: { directoryAndTrailingSlashHandler: true }
  • in your repo pnpm build && pnpm preview and check it is working
  • deploy to GH Pages

I'm preparing the docs, the experimental option added in VitePress cannot be added, Astro will clone the PWA options and cannot be referenced, it will require a change in vite-plugin-pwa.

@userquin
Copy link
Member

userquin commented Oct 27, 2023

If you cannot checkout the new branch, just clone the new branch directly.

EDIT: forgot it, I'll release a new patch version, now we have the experimental flag...

@userquin
Copy link
Member

userquin commented Oct 27, 2023

@felixroos released v0.1.4

Documentation updated:

@felixroos
Copy link

@userquin thanks for the release! I am now using the new flag and it works :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants