Skip to content

Support rewrite request (Similarly to redirect) #7562

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

Merged
merged 37 commits into from
May 22, 2025

Conversation

omerman
Copy link
Contributor

@omerman omerman commented May 2, 2025

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

The feature is done and ready for production.
It should be introduced to v1.14, which might take some time.
In the meantime Im keeping this branch up-to-date with main.

You can use this to have it as part of your project:
pkg-pr

Copy link

changeset-bot bot commented May 2, 2025

🦋 Changeset detected

Latest commit: 6952f27

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

@omerman
Copy link
Contributor Author

omerman commented May 2, 2025

@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
Copy link
Member

wmertens commented May 2, 2025

@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
Copy link
Contributor Author

omerman commented May 3, 2025

@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
Copy link
Member

wmertens commented May 3, 2025

@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
Copy link
Contributor Author

omerman commented May 3, 2025

@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 😅 ?

image

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

@wmertens
Copy link
Member

wmertens commented May 3, 2025

Yes something like that.

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

@omerman
Copy link
Contributor Author

omerman commented May 3, 2025

@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 omerman force-pushed the feature/request-event-rewrite branch from c7e8332 to 4ac925a Compare May 3, 2025 20:56
@wmertens
Copy link
Member

wmertens commented May 3, 2025

Awesome, a day well spent 😊

@omerman

This comment was marked as resolved.

@omerman omerman marked this pull request as ready for review May 4, 2025 15:58
@omerman omerman requested review from a team as code owners May 4, 2025 15:58
@omerman
Copy link
Contributor Author

omerman commented May 4, 2025

@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 omerman force-pushed the feature/request-event-rewrite branch 3 times, most recently from 8454774 to 1ded727 Compare May 4, 2025 16:31
@omerman omerman marked this pull request as draft May 4, 2025 18:21
@omerman
Copy link
Contributor Author

omerman commented May 4, 2025

@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
Copy link
Member

wmertens commented May 4, 2025

@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
Copy link
Contributor Author

omerman commented May 4, 2025

@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.

image

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:
image
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 omerman marked this pull request as ready for review May 5, 2025 18:32
@omerman
Copy link
Contributor Author

omerman commented May 5, 2025

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

@omerman omerman force-pushed the feature/request-event-rewrite branch from 6ca0a77 to 6952f27 Compare May 22, 2025 07:18
Copy link
Member

@wmertens wmertens left a comment

Choose a reason for hiding this comment

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

LGTM

@wmertens wmertens enabled auto-merge (squash) May 22, 2025 14:00
@wmertens wmertens merged commit 7b7e0d7 into QwikDev:main May 22, 2025
19 checks passed
@github-project-automation github-project-automation bot moved this from Waiting For Review to Done in Qwik Development May 22, 2025
@omerman omerman deleted the feature/request-event-rewrite branch May 22, 2025 15:42
@omerman omerman restored the feature/request-event-rewrite branch May 22, 2025 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants