Support rewrite request (Similarly to redirect)

[omerman]

What is it?

Feature / enhancement

Description

This PR adds functionality to support URL rewrites.
At the moment, a user can perform a redirect, which will re-route the request entirely, modifying the url the user sees.
This feature will introduce a new method, rewrite, to the event, alongside the already existing redirect.
rewrite will make an internal redirect, keeping the url of the request intact.

This change addresses qwik-evolution discussion 237 by giving developers explicit control over routing behavior while maintaining SEO best practices.

Checklist

• My code follows the developer guidelines of this project
• I performed a self-review of my own code
• I added a changeset with pnpm change
• I made corresponding changes to the Qwik docs
• I added new tests to cover the fix / functionality

[changeset-bot]

🦋 Changeset detected

Latest commit: d125553

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages

Name	Type
@builder.io/qwik-city	Minor
eslint-plugin-qwik	Minor
@builder.io/qwik	Minor
create-qwik	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[wmertens]

Incidentally, I'd also like us to support location objects for easy parameter changing.

[wmertens]

The response should stay active, no?

[omerman]

Yes, I initially taken the phrasing from redirect - I'm still figuring things out, I'll rephrase it

[omerman]

Fixed

[omerman]

@wmertens At the moment I managed to render a white screen 😅, at least without errors.

async function runNext(requestEv: RequestEventInternal, resolve: (value: any) => void) {
...
if (e instanceof RedirectMessage) {
      const stream = requestEv.getWritableStream();
      await stream.close();
    } else if (e instanceof RewriteMessage) {
      const stream = requestEv.getWritableStream();
      await stream.close();
    }
...

I'm not yet sure where exactly the rewrite can be handled properly.
I treat both the same as a placeholder. but Im not yet sure how to approach it, Im still looking.

Do you have ideas?

[wmertens]

@omerman when you get a rewrite request, you need to restart processing, so you should basically call the handler again, skipping any initialization tasks, and making sure it can't do an infinite loop (e.g. only allow like 5 redirects, that should be plenty)

[omerman]

@wmertens

@omerman when you get a rewrite request, you need to restart processing, so you should basically call the handler again, skipping any initialization tasks, and making sure it can't do an infinite loop (e.g. only allow like 5 redirects, that should be plenty)

So far I hopefully managed to understand some of the flow:

we call runQwikCity with a serverRequest event that in dev-time we generate and in prod we get it from all the possible integrations.
We build all the handlers which are phases the request passes through. we then wait for all them to complete. and stream the result in the end.

I was thinking the key part is in the runQwikCity implementation:

export function runQwikCity<T>(
  serverRequestEv: ServerRequestEvent<T>,
  loadedRoute: LoadedRoute | null,
  requestHandlers: RequestHandler<any>[],
  trailingSlash = true,
  basePathname = '/',
  qwikSerializer: QwikSerializer
): QwikCityRun<T> {
  let resolve: (value: T) => void;
  const responsePromise = new Promise<T>((r) => (resolve = r));
  const requestEv = createRequestEvent(
    serverRequestEv,
    loadedRoute,
    requestHandlers,
    trailingSlash,
    basePathname,
    qwikSerializer,
    resolve!
  );
  return {
    response: responsePromise,
    requestEv,
    completion: asyncStore
      ? asyncStore.run(requestEv, runNext, requestEv, resolve!)
      : runNext(requestEv, resolve!),
  };
}

by re-run the flow, do you mean I should support a way for the responsePromise to wait for another requestEv to be created(createRequestEvent(...)) with the modified path arguments under the same response?

[wmertens]

@omerman so the http request object itself cannot be recreated, and should be retained.

Ideally the whole RequestEvent object is retained, the less work the better.

After the rewrite, the route modules have to be regenerated and the request handling has to happen again.

[omerman]

@omerman so the http request object itself cannot be recreated, and should be retained.

Ideally the whole RequestEvent object is retained, the less work the better.

After the rewrite, the route modules have to be regenerated and the request handling has to happen again.

Do you mean something like this, e.g passing this function to be used somewhere deeper as part of the process, and if rewrite occours rerun the output's built routeHandlers and return the final response? Am I getting close 😅 ?

By regenerate the route modules you meant building the routeHandlers again, right?

[wmertens]

Yes something like that.

Also, very important to add e2e tests. See /e2e

[omerman]

@wmertens It works well now. I will fix up my code making it more readable and better, and will write e2e tests as you asked.
Thank you for all the help 🙏

[omerman]

  // TODO - handle rewrite to urls with search params
  // This is not accurate, because the url might have search params

[wmertens]

Hmm. Ok, I can see rewriting to a string be like a shortcut that passes on search params, and then passing a URL object should not do anything

[omerman]

I made the API as flexible as possible. Let me know if you think it should be different.

[wmertens]

Awesome, a day well spent 😊

[omerman]

TODO - not sure if this is the best name for this function.
it doesnt actually apply the rewrite itself.
Open to suggestions

[omerman]

@wmertens Ready to go. works both in dev and prod environments.

Edit:
I also went over all the requirements, changeset, tests, docs are all ready :)

Let me know if you think something is missing or should be changed.

[omerman]

@wmertens Sorry to bug you yet again, I'm reverting this PR back to a draft because I tried to link it to my work in progress of converting a full website to qwik, and I get an issue with routeLoaders.

Uncaught (in promise) Error: You can not get the returned data of a loader that has not been executed for this request.

I'm guessing its because of the limitation of having route loaders re-export in routes or something like that.

As usual, I'll look into it in depth soon, but if you have any insights that might help Im all ears :)

[wmertens]

@omerman awesome work 🚀

I'm guessing that executing the routeloaders got skipped somewhere. I'm not quite sure if we can keep the results of the previous run but I'm inclined to let that happen for efficiency reasons.

[omerman]

@wmertens Thank you 🙏 hope I'll crack the remaining issue as well.
I have new info to share on that regard, the issue's root is from a resolveValue i'm using at the head.

It would seem that once performing a client navigation, we execute the head code again, which makes perfect sense.
As you mentioned, somehow the routeLoaders didnt run, because the headers were sent (stream got closed)

I did manage to make it work by doing this:

Offcourse it feels redundant (putting the loading of the actions right before the renderQData handler..
(renderQData closes the stream)

I will try to get more clues as to why in this specific case it goes down like this.
Let me know if it rings any bells 🤷

[omerman]

@wmertens I've fixed all the issues. it works perfectly now. the flow is fully operational 🎉

[omerman]

@wmertens If you have any suggestions as to how to make this function clearer, I'm all ears. It became more complex after introduced with the rewrite handling.

[omerman]

@wmertens I fixed and improved the code per your notes, thank you 🙏
Just so we dont miss each other's comments on this long thread here are the highlights:

#7562 (comment)

#7562 (comment)

Aside both comments I did as instructed :)