Skip to content
This repository has been archived by the owner on Jul 15, 2020. It is now read-only.

Project architecture question #89

Open
ghost opened this issue Jul 19, 2018 · 3 comments
Open

Project architecture question #89

ghost opened this issue Jul 19, 2018 · 3 comments

Comments

@ghost
Copy link

ghost commented Jul 19, 2018

Hi,

I'm revisiting this project once again after the change made in next-auth@1.8.0 to include the method for password-based sign in. Really appreciate the work you did there.

I'm still having another problem, which is that I'm trying to merge the Layout and Page components into one higher order component for my project. I have that mostly working, but there is a problem with sessions being initialized twice in the store due to the way Next.js Starter intends for pages to inherit from the Page component. I am hoping a discussion about architectural design decisions might help solve my problems, or at least clear up some of the things that have me wondering why they are that way.

As a bit of backstory here to what I'm trying to accomplish, I have a Page component of my own that I have been using from another project that doesn't deal with sessions and it's worked well so far for layout purposes. In that project I favored composition over inheritance and my pages include a element in their render() method that renders the layout. Because of this, I thought I would want to make my pages extend React.Component instead of the included Page component, and copy that approach. I know your Layout component is used that way but that component felt bloated for my purposes, and I'm trying to merge our approaches basically. My thought is that if I can make a higher order component called Page that renders the layout and adds the session to props that it would work, and later I could split it up into smaller HOCs such as withSession, withI18next, etc. It seems to me this would enable composition of HOCs since some pages may not need session awareness or localization, for example. I could still have a Page HOC that composes both session awareness and localization, however.

In my attempt to do this, I wanted to use getInitialProps and server-render my pages the way I am used to without calling super.getInitialProps() and without lazy loading things on the clientside because I looked at the async data fetching example (pages/examples/async.js) and did not like how AsyncData.getData() is called in both getInitialProps and componentDidMount since that leads to code duplication, or at least extra boilerplate for every piece of data you want to fetch. So I wanted to avoid that and just await data on the server-side in getInitialProps(). That's what I'm used to and how my existing code works that I'm trying to merge the Next.js Starter architecture into and add session/login support to using nextjs-starter's approach which uses next-auth.

I am using a custom Express server with custom routes for my API that I want my pages to fetch data from in getInitialProps. My problem now is that if I navigate to a page which fetches data from the API in getInitialProps, it will initialize a new session in the session store. I'm not sure why this is. I should note that I prefer my routes to be nested Express routers that get used like regular middleware and not the expressApp passing approach used in nextjs-starter. i.e. my server uses my routes like so:
server.use('/api', require('./api'));
and then in 'api.js':
router.get('/some-api-route', (req, res, next) => { res.json({data: 'it works!'}) });
And this enables me to have a hierarchical route structure without repeating the entire path for every GET/POST handler. So the full path to the route above is /api/some-api-route, but in api.js I don't have to specify the full path (just '/some-api-route').

As an aside regarding the higher order component idea, I am trying to move towards a higher order component for my pages, but I am not sure if using HOCs is a good idea in general and I am wondering if there is a way to use render props instead to achieve everything that that the higher order component(s) would do. For example, I was thinking I could have a withSession higher order component, as well as a withI18next higher order component, etc. But I know that render props can do anything a higher order component can do and are more flexible and versatile. I wanted to mention this and see if HOCs and/or render props are a direction Next.js Starter might go in.

Sorry if that's a big wall of text and I don't have a single clear and concise question for you, but if you could respond to some of those general points that might help clear up some confusion. I am also hoping to open a discussion surrounding the architectural decisions this project has taken, as I may not be the only one with these questions.

My time is limited right now but if I find time soon I might create an open repository with some code in the direction I'm trying to move towards, or simply fork this repository. If so I will update this issue with the link.

Thank you.

@ghost
Copy link
Author

ghost commented Jul 19, 2018

Update:

I've noticed this extra session initialization happens in Next.js Starter whenever a request is made from getInitialProps to a route that is in the same expressApp that Next.js is handling requests to. I noticed this just by adding a route to the custom server:
expressApp.get('/test', (req, res, next) => { res.json({data: 'some data'}); });

and updating async-data.js to fetch data from
http://localhost:3000/test instead of //jsonplaceholder.typicode.com/posts

I noticed there is the following quote in nextauthjs/next-auth#14 (comment):

A session in a databases will only be created for a user when they log in, to reduce database load - this also helps provide some protection against trivial Denial of Service attacks.

Could this be related?

Is the recommended approach here to make a new Express instance for my API?
What if I want to protect certain API routes based on the contents of req.user in my routes?
I read through some of the suggestions in #12 but what if I want my API route to output different data (or no data at all) based on the presence of req.user or some property of req.user?

I had a similar thing like that working when I was trying to roll my own Next.js and Express-based authentication solution before trying Next.js Starter so it has to be possible. Perhaps I could dig up that code. The reason I switched to Next.js Starter was out of client-side security concerns, cookie handling, and because I ran into session duplication on the Express API server side.

Edit: I think I need to look at the admin route again... It seems to do restrict access exactly as I've described. I'm still confused as to why sessions are getting initialized when I try to GET from my API route, though.

Edit 2: It seems the admin page does not implement getInitialProps() which is why it doesn't cause a new session to be initialized. Attempting to implement getInitialProps() causes it to return 403 Forbidden. I think this is because the server does not keep track of the user's authenticated state.
I know when using Redux I heard somewhere on the server we have to have a Redux store for each client. I guess this is similar.

I guess to make a long question short, are there any plans to allow this, or create an example of this? Or am I doing something wrong?

@ghost
Copy link
Author

ghost commented Jul 19, 2018

I'm wondering if my approach of awaiting data from my own API in getInitialProps even makes sense to do. I've been doing that for several Next.js projects. But if it's my own API on the same custom server as Next.js, is there even a point to that?

I guess my first inclination towards doing so is to server-render the pages with the data already in them to prevent flashing the page without data on it. My second inclination towards doing so is to enable dynamic content to be returned from the API routes while passing the request through all appropriate Express middleware, for example, for access restriction and logging. If I were to not do it, I could simply import the API route function in my clientside code and call it rather than make a request to the server from itself. But then we would lose the server-controlled access restriction and logging. That is perhaps more important.

Still not sure what to do here. If I create a separate Express instance just for the API, I would be required to provide a token to the API server from the client in order to authenticate, which is something I was hoping next-auth would solve for me by adding the user property to the req object.

@ghost
Copy link
Author

ghost commented Jul 20, 2018

I figured out how to send cookies to the expressApp using

const res = await axios({
	url: `http://localhost:3000/api/my-api-route`,
	headers: req ? {cookie: req.headers.cookie} : undefined, // Manually copy cookie on server, let browser handle it automatically on client
});

inside getInitialProps and receiving the cookie in Express using cookie-parser:

expressApp.use(require('cookie-parser')());

I then realized getInitialProps only executes on the server for the initial page load, whereas when navigating to a different route via the Link component or using the routing APIs it will only be executed on the client. Thus we still/also need to make the request on the client. I was able to achieve this by letting getInitialProps send the request on both the server and client (no checking if (!!req) in getInitialProps which limits it to running on the server).

Once I got it working with Axios, I was able to convert it to isomorphic-unfetch and isomorphic-fetch per vercel/next.js#2455 (comment). It turns out the two are very similar in how they request the cookie to be sent in the header. I'm still not sure what the difference is between isomorphic-unfetch and isomorphic-fetch. I think I will stick with isomorphic-fetch...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

0 participants