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

React Routing #38

Open
marco-dasilva opened this issue Jan 30, 2020 · 29 comments
Open

React Routing #38

marco-dasilva opened this issue Jan 30, 2020 · 29 comments
Labels
question Further information is requested

Comments

@marco-dasilva
Copy link

I usually like to do my research and spend a lot of time doing so before reaching out for help but I'm at a dead end. I must be having tunnel vision at this point but either way here goes.

I understand the reason for handling routes in Nest and routing them to Next but I'm actually looking for Next to handle anything not defined in Nest. I don't know if this is the correct use case but here's an example none the less.

In pages/, I create a file called index.tsx and in nest I add to the AppController a Get('') and Render('Index'). This loads on localhost:3000. In this index.tsx file, I create a link to redirect the user to a new page called Profile (). Clicking on this link for Profile, it successfully routes the user however when I refresh, Nest kicks in and throws an error.

Is there a solution for this?

In conclusion, if I go to /asadd, a non existent route in Nest, I would like to see the Next 404 error page and not the Nest stack trace page.

@kyle-mccarthy
Copy link
Owner

kyle-mccarthy commented Jan 31, 2020

So I haven't actually tried this before, but messed around and think I found a solution. You will want to create your own global ExceptionFilter that passes the request and response to Next rather than handling it in Nest.

I was able to get it working with the following code. Some things to keep in mind is that this will pass on all of your NotFoundExceptions to Next, depending on your use case this should probably be fine. Also it will route based on the filesystem path, if you have a some route /test it resolves to pages/test.tsx.

Let me know if this works for you!

import {
  ArgumentsHost,
  Catch,
  ExceptionFilter,
  HttpException,
  Inject,
  NotFoundException,
} from '@nestjs/common';
import { NestFactory } from '@nestjs/core';
import { RenderModule, RenderService, RequestHandler } from 'nest-next';
import Next from 'next';
import 'reflect-metadata';
import { AppModule } from './application.module';

// Additional global filter to pass 404 errors onto next rather than
// handling in nest.
@Catch(NotFoundException)
export class NextPageFilter implements ExceptionFilter {
  private requestHandler?: RequestHandler;

  constructor(@Inject() private readonly renderService: RenderService) {
    this.requestHandler = this.renderService.getRequestHandler();
  }

  catch(exception: HttpException, host: ArgumentsHost) {
    const ctx = host.switchToHttp();
    const res = ctx.getResponse();
    const req = ctx.getRequest();

    if (this.requestHandler) {
      return this.requestHandler(req, res);
    }

    // the requestHandler technically should always be defined if the nest-next
    // was correctly setup - rethrow just in case
    throw exception;
  }
}

async function bootstrap() {
  const dev = process.env.NODE_ENV !== 'production';
  const app = Next({ dev });

  await app.prepare();

  const server = await NestFactory.create(AppModule);

  const renderer = server.get(RenderModule);
  renderer.register(server, app);

  const service = server.get(RenderService);
  server.useGlobalFilters(new NextPageFilter(service));

  await server.listen(3000);
}

bootstrap();

@marco-dasilva
Copy link
Author

I see it hitting the constructor but it isn't hitting the catch exception - I had a similar implementation of this earlier from a stackoverflow thread so I'll see what I missed there. I am confident I setup nest-next correctly with fastify.

Here's an example screenshot of what I'm seeing without defining the render in Nest. I used your basic example from the repo.
cannot-get-object

I am not sure why I didn't think of this but checking the source code of the served page Cannot GET /test shows next delivering it with a stack trace but for some reason when I setup the following code in Nest and then attempt to hit /test I get the actual 404 that I wanted:

@Render('test')
@Get('/test')

404

@kyle-mccarthy
Copy link
Owner

Could you try updating the code for that filter to call the request handler like return this.requestHandler(req, res);. I noticed a problem with the query not being what next was expecting.

Also I just wanted to confirm two things:

  1. Make sure that you attach the global filter to the server after registering the RenderModule
const renderer = server.get(RenderModule);
renderer.register(server, app);

const service = server.get(RenderService);
server.useGlobalFilters(new NextPageFilter(service));
  1. The test file's file location is /pages/test.tsx, it shouldn't be in the views folder. Also the naming is case sensitive so it should be lowercase.

@marco-dasilva
Copy link
Author

marco-dasilva commented Jan 31, 2020

Good point! For (1) above, the order mattered - I was incorrectly setting it before it registered. Once it hit the filter, now seeing:
The "url" argument must be of type string Received type undefined

@marco-dasilva
Copy link
Author

I can confirm it's when using fastify.

@marco-dasilva
Copy link
Author

Solved the issue by returning this on NextPageFilter:

return this.requestHandler(res.request.raw, res.res);

@kyle-mccarthy
Copy link
Owner

ah yeah, I forgot that fastify modifies the req and res. I believe that you can also do

return this.requestHandler(req.raw, res.res)

That is how we handled it in render.filter.ts FWIW.

@jgoux
Copy link

jgoux commented Jun 18, 2020

This is exactly my use-case, I was actually surprised that it isn't the default.

Do you think this could be included and enabled via an option?

I used your solution and it works, except when NestJS is reloading with its incremental compilation, I got this error :

(node:16097) UnhandledPromiseRejectionWarning: Error: ENOENT: no such file or directory, rmdir '/home/jgoux/Projects/oss/typezafe/packages/web/.next/static/chunks'
(node:16097) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:16097) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

For the error to disappear, I need to delete the .next folder. 🤔
I use NestJS cli nest start --watch command to launch my dev script.

Maybe there is a conflict between the RenderModule and the NextPageFilter? Because I am calling Next() in both of them.

@kyle-mccarthy
Copy link
Owner

@jgoux I think that should be possible. Just to clarify, you want all the requests that Nest thinks are 404s to be handled by Next's request handler? The only problem that I could see with this, is even if you throw a NotFoundException it would still be forwarded to nextjs. If we were to go this route I think it would resolve #47 and #48.

Also I believe that calling Next tries to start the server, so the exception being thrown is probably because it sees something using the port. Can I ask why you're calling it from NextPageFilter?

@kyle-mccarthy kyle-mccarthy reopened this Jun 18, 2020
@jgoux
Copy link

jgoux commented Jun 18, 2020

@kyle-mccarthy Sorry I meant I'm callling it from both bootstrap and the RenderModule.forRootAsync in my AppModule.

So it seems to start the Next.js compilation twice in parallel.

If we use the NextPageFilter approach, should we do anything special to the AppModule?

import path from "path";
import { Module } from "@nestjs/common";
import { RenderModule } from "nest-next";
import Next from "next";
import { AppController } from "./app.controller";
import { PrismaService } from "./prisma/prisma.service";

@Module({
  imports: [
    RenderModule.forRootAsync(
      Next({
        dev: process.env.NODE_ENV !== "production",
        dir: path.resolve(__dirname, "../../web"),
      }),
      {
        viewsDir: null,
      }
    ),
  ],
  controllers: [AppController],
  providers: [PrismaService],
})
export class AppModule {}

If I omit to import RenderModule in this file, I got this error :

Error: Nest could not find RenderModule element (this provider does not exist in the current context)

for the line const renderer = server.get(RenderModule); in boostap.

Sorry it's maybe obvious but I'm beginning with NestJS ^^

Just to clarify, you want all the requests that Nest thinks are 404s to be handled by Next's request handler?

Yes this is exactly what I want. :)

@kyle-mccarthy
Copy link
Owner

@jgoux Oh yeah so this issue is a little older and uses the deprecated bootstrapping method - you don't need to call it in your bootstrap file anymore. See https://github.com/kyle-mccarthy/nest-next/blob/master/examples/basic/src/main.ts

Also to follow up on my question, are you wanting to have all 404s be handled by next instead of nest handling it? This is a feature I would definitely be open to adding, and it could be controlled through an option like you suggested.

@jgoux
Copy link

jgoux commented Jun 18, 2020

Yes I don't mind having next handling all the 404s. What could be the downside?

As a total outsider I really thought this was the default behaviour, and that you could add specific Nest handlers as you go.

Just to be sure, following the modern bootstraping approach, is this the correct way to register the NextPageFilter? Or can it be moved to AppModule as well?

async function bootstrap() {
  const server = await NestFactory.create<NestFastifyApplication>(
    AppModule,
    new FastifyAdapter()
  );

  const service = server.get(RenderService);
  server.useGlobalFilters(new NextPageFilter(service));

  await server.listen(3000);
}

@kyle-mccarthy
Copy link
Owner

That is correct, since you need to provide the RenderService for your filter.

And I don't really see any issue with passing the request to next when nest 404s, I will look into providing that option.

@jgoux
Copy link

jgoux commented Jun 18, 2020

This is awesome, thanks a lot for your answers and your work on this library! The NestJS ecosystem really seems great. 😄

@dayrim
Copy link

dayrim commented Jun 19, 2020

Any chance you can provide an example project for this ?

@kyle-mccarthy
Copy link
Owner

@jgoux @dayrim I've made the updates that will allow for you passthrough requests that 404 to next. It's currently on the next branch and you can preview it by doing a yarn add nest-next@beta. There is also an example available here https://github.com/kyle-mccarthy/nest-next/tree/next/examples/passthrough

I've still got to update the main docs, but there are only two changes required to get started.

  1. Update the dir property in next's config dir: resolve(__dirname, '..')
  2. Enable the passthrough404 option in the nest-next config

Both of those are highlighted in the example's ApplicationModule.

As a side note, I couldn't think of a great name for the option so I took a pretty literal approach. LMK if you think there is a name that better describes the feature.

@jgoux
Copy link

jgoux commented Jun 23, 2020

Thanks for the change, I'll test it soon!

Is there any way to support src/pages as the path? Next.js supports both pages and src/pages as valid paths, and I prefer to group everything in src.

For the naming, maybe nextFallback? I don't know, naming is the hardest part. 😄

@kyle-mccarthy
Copy link
Owner

@jgoux I actually think that should be possible by omitting step 1 from my previous comment. The only reason I actually did that is so that next knows it should look in the src's parent directory for the pages dir.

@kyle-mccarthy
Copy link
Owner

@jgoux Did you have an opportunity to try this out? If it works for your use case, I can merge this and deploy a new version.

@jgoux
Copy link

jgoux commented Jul 29, 2020

@kyle-mccarthy No sorry I was on another subject. I'll try it this week and let you know. Thanks again for your patience.

@twigs67
Copy link

twigs67 commented Sep 10, 2020

@kyle-mccarthy I'm getting the following error:

Argument of type '{ passthrough404: boolean; viewsDir: null; }' is not assignable to parameter of type 'Partial<RendererConfig>'.
  Object literal may only specify known properties, and 'passthrough404' does not exist in type 'Partial<RendererConfig>'.

Any idea how I can get this working?

Thanks

@kyle-mccarthy
Copy link
Owner

@twigs67 hey thanks for checking this out - I just want to make sure that you have the right version installed since this currently isn't mastered. Did you add nest-next with yarn add nest-next@beta?

@twigs67
Copy link

twigs67 commented Sep 10, 2020

@kyle-mccarthy I did not. Sorry about that - should have read a bit more. Thanks!

@kyle-mccarthy
Copy link
Owner

@twigs67 no problem at all - I actually do want to master this ASAP but have been awaiting feedback. If it works for your use case and works how you'd expect let me know and I'll master it + deploy a new version.

@twigs67
Copy link

twigs67 commented Sep 10, 2020

@kyle-mccarthy yes, it did work for me. However, I had to remove viewsDir as it was giving me a 404.

@kyle-mccarthy kyle-mccarthy added the question Further information is requested label Sep 30, 2020
@ralls
Copy link

ralls commented Apr 12, 2022

@kyle-mccarthy do you plan on releasing 9.3.0-beta.0? It's the only way I am able to implement Auth0.

Edit: I did not have to remove the viewsDir option, it seems to work just fine.

@kyle-mccarthy
Copy link
Owner

@ralls done! just deployed v10 to npm

@ralls
Copy link

ralls commented Apr 13, 2022

@kyle-mccarthy Thank you! 👍

@joshkay
Copy link

joshkay commented Sep 6, 2022

Is it possible to filter which 404 reponses are passed through to next? I would like to prevent my 404 api responses from rendering an error page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

7 participants