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

🐛 BUG: Removal of .html Extension Leads to Issue with Angular Service Worker Registration and Client-Side Navigation #5262

Open
naveedahmed1 opened this issue Mar 15, 2024 · 25 comments
Labels
bug Something that isn't working internal Requires support from the Cloudflare Platform pages Relating to Pages

Comments

@naveedahmed1
Copy link

Which Cloudflare product(s) does this pertain to?

Pages

What version(s) of the tool(s) are you using?

wrangler: 0.0.0-0c535e72

What version of Node are you using?

v20.11.1

What operating system and version are you using?

Windows 11

Describe the Bug

Observed behavior

Currently, the platform automatically maps index.html to /, disrupting the expected behavior of Angular's service worker and client-side navigation for subsequent requests.

Current Behavior
The automatic mapping of index.html to / leads to issues with Angular's service worker and client-side navigation. When the Angular service worker initializes and attempts to fetch index.html, it receives a server-side rendered (SSR) response of /. This SSR response does not match the hash value expected by the service worker, causing a hash mismatch error and preventing the service worker from being registered. Attempts to mitigate this issue by renaming the index.html file in the Angular service worker configuration or by adding .html twice to the file name only partially resolve the problem, as page refreshes still result in requests to the server instead of to index.html on the client.

Expected behavior

Requests for index.html should return the contents of the index.html file without any redirection.

Steps to reproduce

To observe this behavior, visit https://www.mustakbil.com/. Upon the first request, you will receive a server-rendered response that registers the service worker. However, on subsequent page reloads, the request continues to go to the server instead of to index.html on the client.

Additional Information
Several users have reported similar issues, including discussions within the Cloudflare community:

Cloudflare Pages Truncates URLs by Removing the HTML Extension
Prevent Truncating and Removal of Page Name Extensions

and more:

https://community.cloudflare.com/search?q=%22.html%22%20%23developers%3Acloudflare-pages

The issue has been discussed with Adam Murray, @petebacondarwin, and @alan-agius4 from the Angular team. Alan Agius suggests that this seems to be more of a problem with Cloudflare and recommends filing an issue with them. He also emphasizes the importance of the file extension, as its absence can lead to invalid paths and build failures.

Here's what @alan-agius4 from Angular team says on the issue:

This seems more of a problem in Cloudflare. I suggest filing an issue with them.
The file extension is needed as otherwise it will become an invalid path and cause the build to fail.

Please provide a link to a minimal reproduction

No response

Please provide any relevant error logs

No response

@penalosa
Copy link
Contributor

penalosa commented Jul 8, 2024

@tanushree-sharma

@penalosa penalosa added pages Relating to Pages internal Requires support from the Cloudflare Platform labels Jul 22, 2024
@petebacondarwin
Copy link
Contributor

There is no plan to address this in Pages, but we are working to add asset hosting to Workers in general and we will ensure that this is fixed one way or another there.

@naveedahmed1
Copy link
Author

@petebacondarwin is there any update on this?

@petebacondarwin
Copy link
Contributor

Hey @naveedahmed1 - thanks for being patient with us. Last week we announced new support for Static Asset hosting for normal Workers, avoiding the need to use Pages when hosting assets. Moreover this support provides the ability to configure how we handle rewriting/redirecting of "index" Pages, and HTML requests with extensions. Take a look at https://developers.cloudflare.com/workers/static-assets/routing/. I believe this should provide you with a way forward.
You can create a new Angular app that uses Workers Assets via our C3 application to try it out (although it is pretty straightforward to convert a Pages app to Workers).

pnpx create-cloudflare@latest --experimental --framework=angular

@naveedahmed1
Copy link
Author

naveedahmed1 commented Sep 30, 2024

Thanks @petebacondarwin I just tried it and deployed the sample app. But it seems that the request to index.html are till being redirect to the root directory.

Incoming Request: /index.html
Response: 307 to /

Incoming Request: /index.csr.html
Response: /index.csr

Apparently the only thing changed is the status code, with previous version it was 308 Permananet Redirect and now its 307 Temporary Redirect.

The requirement is to return the actual static html file if it exists without removing extension and without any redirect e.g.

Incoming Request: /index.html
Response: /index.html

Can you please confirm it from your side?

@tanushree-sharma
Copy link

Did you configure the html_handling option in your binding? You're looking for the html_handling = "none" option that's documented: https://developers.cloudflare.com/workers/static-assets/routing/#html_handling--auto-trailing-slash--force-trailing-slash--drop-trailing-slash--none

This will serve the asset the incoming path without removing the extension or any redirects.

@naveedahmed1
Copy link
Author

Thanks @tanushree-sharma this seems to be working fine.

One quick question, in order to update an existing Angular Project generated using the olrder version of the create cloudflare command, we need these three things:

  1. Add wrangler.toml in root of the project.
  2. Replace the contents of /tools/alter-polyfills.mjs with the content of /tools/alter-polyfills.mjs in new template.
  3. Update @cloudflare/workers-types and wrangler in new project.

And now when we deploy the project it will automatically convert existing Pages project to this new Static Asset Worker project, correct?

@petebacondarwin
Copy link
Contributor

It will not convert the Pages project to a Workers one. It will deploy a new Workers project, with the same name. The old Pages project will still be there and there is no link between the old and new. There is currently no "migration" tool in terms of the backend.

@naveedahmed1
Copy link
Author

I see, and the steps I mentioned above are the only changes I need to make, correct?

@petebacondarwin
Copy link
Contributor

petebacondarwin commented Oct 1, 2024

I believe the steps to convert a standard Angular SSR app to run on Workers + Assets you need are:

  • install wrangler, @cloudflare/workers-types
  • uninstall @angular/ssr, express and @types/express
  • update app.config.ts:
    import { provideHttpClient, withFetch } from '@angular/common/http'
    ...
    providers: [provideHttpClient(withFetch()), ...]
    
  • Update angular.json:
    "projects": { "<project-name>": { "architect": { "build": { "options": { "outputPath": "dist" ... } } } } }
    
  • Add server.ts:
    import { renderApplication } from "@angular/platform-server";
    import bootstrap from "./src/main.server";
    
    interface Env {
      ASSETS: { fetch: typeof fetch };
    }
    
    // We attach the Cloudflare `fetch()` handler to the global scope
    // so that we can export it when we process the Angular output.
    // See tools/bundle.mjs
    async function workerFetchHandler(request: Request, env: Env) {
      const url = new URL(request.url);
      console.log("render SSR", url.href);
    
      // Get the root `index.html` content.
      const indexUrl = new URL("/index.html", url);
      const indexResponse = await env.ASSETS.fetch(new Request(indexUrl));
      const document = await indexResponse.text();
    
      const content = await renderApplication(bootstrap, {
        document,
        url: url.pathname,
      });
    
      // console.log("rendered SSR", content);
      return new Response(content, indexResponse);
    }
    
    export default {
      fetch: (request: Request, env: Env) =>
        (globalThis as any)["__zone_symbol__Promise"].resolve(
          workerFetchHandler(request, env),
        ),
    };
    
  • Add wrangler.toml:
    name = "<project-name>"
    compatibility_date = "<today's-date>"
    main = "./dist/server/server.mjs"
    assets = { directory = "./dist/browser", binding = "ASSETS", html_handling = "none" }
    
  • Add tools/alter-polyfills.mjs:
    import fs from "node:fs";
    import { EOL } from "node:os";
    import { join } from "node:path";
    import path from "node:path";
    import { fileURLToPath } from "node:url";
    const dirname = path.dirname(fileURLToPath(import.meta.url));
    
    /**
     * Split by lines and comment the banner
     * ```
     * import { createRequire } from 'node:module';
     * globalThis['require'] ??= createRequire(import.meta.url);
     * ```
     */
    const serverPolyfillsFile = join(
      dirname,
      "../dist/server/polyfills.server.mjs"
    );
    const serverPolyfillsData = fs
      .readFileSync(serverPolyfillsFile, "utf8")
      .split(/\r?\n/);
    
    for (let index = 0; index < 2; index++) {
      if (serverPolyfillsData[index].includes("createRequire")) {
        serverPolyfillsData[index] = "// " + serverPolyfillsData[index];
      }
    }
    
    // Add needed polyfills
    serverPolyfillsData.unshift(`globalThis['process'] = {};`);
    
    fs.writeFileSync(serverPolyfillsFile, serverPolyfillsData.join(EOL));
    
  • Update package.json scripts:
    "start": "npm run build && wrangler dev",
    "build": "ng build && node ./tools/alter-polyfills.mjs",
    "deploy": "npm run build && wrangler deploy",
    

@naveedahmed1
Copy link
Author

Thanks @petebacondarwin! Please also add a section that guides on migration of existing Angular Pages project with SSR to Workers.

@irvinebroque
Copy link
Contributor

already on it :) cloudflare/cloudflare-docs#17251

@naveedahmed1
Copy link
Author

Awesome! Thanks @irvinebroque

@naveedahmed1
Copy link
Author

naveedahmed1 commented Oct 1, 2024

Angular Worker template seems broken. Try switching the prerender flag to false in angular.json and it will break the app:

"prerender": false,

@petebacondarwin
Copy link
Contributor

@naveedahmed1 - I had a quick play and I found that I needed to set the html handling in wrangler.toml. For example:

assets = { directory = "./dist/browser", binding = "ASSETS", html_handling = "none" }

Otherwise the request for / goes directly to the index.html asset and the Worker is never triggered.

@petebacondarwin
Copy link
Contributor

And if you have prerender turned off then the index.html file is not generated. Instead you need to load the CSR index file:

  // Get the root `index.html` content.
  const indexUrl = new URL('/index.csr.html', url);
  const indexResponse = await env.ASSETS.fetch(new Request(indexUrl));
  const document = await indexResponse.text();

@petebacondarwin
Copy link
Contributor

I'll update the C3 template and the docs...

@naveedahmed1
Copy link
Author

Thanks @petebacondarwin I tried the changes your suggested on the test project generated through the c3 and its seems to be working fine.

Updating C3 template and the docs would be awesome.

Would it be possible to update C3 to throw more dev friendly errors? that allow us to figure out such issues on our own.

@naveedahmed1
Copy link
Author

Can you please also guide how _routes.json would work in this new structure? For example in case of Pages, I added _routes.json to exclude urls for which I want to disable the Server Side Rendering.

@naveedahmed1
Copy link
Author

One more thing which I noticed is that, Cache Rules no longer work with this new approach probably due to this:

Cloudflare Workers run before the cache but can also be utilized to modify assets once they are returned from the cache. Modifying assets returned from cache allows for the ability to sign or personalize responses while also reducing load on an origin and reducing latency to the end user by serving assets from a nearby location.

https://developers.cloudflare.com/workers/reference/how-the-cache-works/

I'm afraid this would be a dealbreaker for us to use new Worker Asset Hosting model :(

Any chance developers could be given an option to select if they want the worker to execute after cache at least when using Worker Asset Hosting?

@tanushree-sharma
Copy link

We don't have a _routes.json equivalent yet. Can share why you want to disable SSR on some routes?

I'm afraid this would be a dealbreaker for us to use new Worker Asset Hosting model :(

Tell us more about what you're building, why this is a blocker.

@naveedahmed1
Copy link
Author

naveedahmed1 commented Oct 8, 2024

We need to diisable the SSR for routes like the account urls. It doesnt make sense to server render such routes since these routes require user to login and are private to the user.

We want to be able to cache the server rendered response. Its an expenive task since it requires to pull the data from the backend api and then generate the page. Many of these pages dont change often so its better to have the response cached, slow respone impact both user expereince as well as the SEO.

@naveedahmed1
Copy link
Author

@petebacondarwin it seems that the angular template is still broken when uisng "prerender": false,:

I'll update the C3 template and the docs...

@naveedahmed1
Copy link
Author

@petebacondarwin just wanted to point out that the index file should point to /_worker.js/index.server.html and not /index.csr.html, using /index.csr.html will break the Angular Incremental Hydration.

So, when you update the template please ensure that index file is pointing to the index.server.html file.

  // Get the root `index.html` content.
  const indexUrl = new URL('/_worker.js/index.server.html', url);
  const indexResponse = await env.ASSETS.fetch(new Request(indexUrl));
  const document = await indexResponse.text();

@naveedahmed1
Copy link
Author

In fact it seems that when deployed, the server.ts dont have acces to /_worker.js/index.server.html, so we need to copy index.server.html to cloudflare folder. And use this instead:

  const indexUrl = new URL('/index.server.html', url);
  const indexResponse = await env.ASSETS.fetch(new Request(indexUrl));
  const document = await indexResponse.text();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something that isn't working internal Requires support from the Cloudflare Platform pages Relating to Pages
Projects
Status: Other
Development

No branches or pull requests

5 participants