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

App directory support with servercomponents #442

Closed
joellp opened this issue Feb 15, 2023 · 24 comments
Closed

App directory support with servercomponents #442

joellp opened this issue Feb 15, 2023 · 24 comments
Labels
area: next-drupal enhancement New feature or request

Comments

@joellp
Copy link

joellp commented Feb 15, 2023

Hello! I'm trying to get next-drupal working with the new app directory of Next.js. This does work for client components, but I cannot get it working for ServerComponents.

import { drupal } from "lib/drupal"
import { NodeArticle } from "components/node--article"
import { DrupalNode } from 'next-drupal';

const Article = async () => {
  // This requires "use client"
  const drupalContent = await drupal.getResource<DrupalNode>('node--article', 'bb601c1a-6e1f-4e09-9b42-ef07136963f4')
 
  return (
    drupalContent ? <NodeArticle node={drupalContent} />: <h1>Hi</h1>
  );  
}

export default Article;

When fetching resources a call to createContext is made, which requires me to use a client component. Is there a way to use this in Server Components?

@shadcn
Copy link
Collaborator

shadcn commented Feb 15, 2023

Not yet. But I'm actively looking into it. I'll post an update soon.

@MokDevelopment
Copy link

I think the app folder is not really usable yet. Not even ReactQuery seems to be usable … nor there is a best practice for language support. Somehow have the feeling the nextJs community tries to ignore the feature as long as pos ;)

@marcinmaruszewski
Copy link

marcinmaruszewski commented Feb 23, 2023

It looks like this piece of code is using the next/router https://github.com/chapter-three/next-drupal/blob/main/packages/next-drupal/src/use-menu.tsx#L2. As far as I know NextJS this component is trying to get the current locale.

I did some investigation and the error occurs at the moment when some of the next-drupal methods are used. And it's because the use-menu.tsx is always included - https://github.com/chapter-three/next-drupal/blob/main/packages/next-drupal/src/index.ts#L11 included. This is how the webpack works.

The next/router is by use-menu.tsx in order to guess the current language context as part of NextJS i18n support.

When I started searching the web for some examples of i18n with NextJS 13 and the app directory I found this article - https://dev.to/ajones_codes/the-ultimate-guide-to-internationalization-i18n-in-nextjs-13-ed0. TLDR with the new app directory you have to handle i18n yourself in the middleware. This means that this repository should drop next/router and try to use the middleware to adapt to the new approach. But AFAIK that would be a Breaking Change.

There is also a second way. I have studied https://next-drupal.org/docs/fetching-resources and I see that i18n support is provided by the developer using the options.locale and options.defaultLocale parameters. IMHO we could remove support by reverting this commit 9463bd1. But I don't know what was the point of including this change. And it would also be another Breaking Change.

I hope that Chapter Three will take a closer look at this in the near future.

@JohnAlbin
Copy link
Collaborator

Next.js 13.4 was just released and the app directory router is no longer "beta"; it is a stable API. See the blog announcement here: https://nextjs.org/blog/next-13-4

My current project requires server-side rendering using Next.js' generateStaticParams and next-drupal can't be imported into a file that uses that Next.js function.

@amarincolas
Copy link

Any update on this matter @shadcn?

@trickreich
Copy link

@shadcn have you started with a prototype already and/or could we help with implementing this?

@taskrider2022
Copy link

@shadcn I am also very interested in this feature. Is there any info about it?

@sinyayadynya
Copy link

More and more people seems to be interested... @shadcn Maybe just to have an idea about any expected timeline for the next next-drupal update would be helpful.

@robdecker
Copy link
Member

Adding app directory support (and all the implications that go along with it) is on our roadmap, but we don't have an eta yet.

@trickreich We are open to contributions if anyone is so inclined.

@MathiasGmeiner
Copy link

A while back, @shadcn mentioned in the #nextjs Drupal-Slack channel that the work has already started. @robdecker, could you share that progress as a starting point for those who are willing to participate? Thanks for your work so far!

@shrop
Copy link

shrop commented Sep 20, 2023

Curious on a status update for this issue. Vercel is recommending App Router for some of our use cases.

@JohnAlbin
Copy link
Collaborator

JohnAlbin commented Oct 4, 2023

@marcinmaruszewski’s comment above is exactly the issue.

It looks like this piece of code is using the next/router

it's because the use-menu.tsx' is always included

Yep. use-menu.tsx is using a client-side-only piece of Next.js' APIs.

There's nothing wrong with the code per se. The issue is that all of next-drupal is bundled into a single JavaScript module so you always import that client-only code even if you don't want to use it.

The quick fix here is to either change how microbundle bundles or to not bundle at all and just compile the multiple, existing TypeScript modules into multiple JS modules.

This fix is related to #528 [edit: nah. that issue can be done later.]

I'm working on a PR for this. And hoping to get it done before/during Drupalcon Lille.

@JohnAlbin
Copy link
Collaborator

JohnAlbin commented Oct 17, 2023

I experimented with switching from microbundle to just using tsc to build the JavaScript. While that does work, it means that all the files in src become entry points.

For example:

import { DrupalClient } from 'next-drupal/client';
// ick.
import { useMenu } from 'next-drupal/use-menu';
import { getMenu } from 'next-drupal/get-menu';

And, frankly, none of those files names were meant to be entry points into the package, so I don't think this swap to tsc works.

Looking more closely at microbundle, I see it has the ability to specify multiple entry points, so I'm experimenting with that now. I hope to have an experimental PR out later today to celebrate the first day of Drupalcon Lille.

The fix for being App Router-compatible, will be to use the new entry points instead of the main entry point.

Current import code (that will continue to work in any 1.x release):

// Throws error in Server Components since it also imports useMenu().
import { DrupalClient } from 'next-drupal';

New import code:

// Can be used on Server or Client components.
import { DrupalClient } from 'next-drupal/client';

// Can only be used in Client components.
import { useMenu } from 'next-drupal/navigation';

Does anyone have any comments/concerns with this path? For example, do we need to add App Router support to codebases using next-drupal 1.0’s get[DrupalThing] functions? Hmm…

@Danishkhurshid
Copy link

Danishkhurshid commented Oct 17, 2023

@JohnAlbin I'm eager to find out your results with micro bundling. i have gone down the same path but ran into issues while microbundling with multiple entry points.

I'm also curious about whether it's possible and advisable to make 'next-drupal' the default entry point for exports that can be used in both client and server components.

So rather than doing this:

// Can be used on Server or Client components.
import { DrupalClient } from 'next-drupal/client  // the client export sounds like it can only be used in client components. 

you could simply use this in components that are compatible with both client and server:

import { DrupalClient } from 'next-drupal'

And this with components that can only be used client side.
import { useMenu } from 'next-drupal/client';

@JohnAlbin JohnAlbin added enhancement New feature or request area: next-drupal labels Oct 17, 2023
@JohnAlbin
Copy link
Collaborator

// the client export sounds like it can only be used in client components.
import { DrupalClient } from 'next-drupal/client

Yeah, the "client" part of the path is mildly confusing. The "client" in DrupalClient is meant to connect to the "Drupal Server" (PHP), so that's where the name came from. So connecting to the Drupal server from a Next.js Server Component will still require the DrupalClient. We're stuck with that name until a major refactor is done. And I don't want to do a major refactor when a small fix will fix this issue.

you could simply use this in components that are compatible with both client and server:

import { DrupalClient } from 'next-drupal'

Yep, but that's a breaking change.

The problem we also need to solve is that Next.js specifically designed the App Router to be compatible with the pages/ directory so that large code bases could be migrated from the old router system gradually.

So there are two paths to fixing that:

  1. Breaking change; release 2.0.0. Copy all 1.x exports to a new entry point (maybe import {} from 'next-drupal/deprecated'?)

    Users will be required to update all existing files to change from "next-drupal" to from "next-drupal/deprecated so that old code doesn't break. New code can use the new entry points and the default entry point only contains things that are compatible with both Server and Client Components.

  2. Release 1.7.0 that keeps the old entry points and exports in-place. Add new entry points that can be used for new code for the App Router. And then we need to plan a full migration plan for existing codebases.

I feel like option 1 is too disruptive a change.

Option 2 may require a disruptive change later (when 2.0.0 comes out), but that will come with major refactoring where the change will feel like it's worth it.

i have gone down the same path but ran into developit/microbundle#50 while microbundling with multiple entry points.

Thanks for the tips! I'll read over that.

@kmajzlik
Copy link

kmajzlik commented Oct 17, 2023

New features should come as new minor version. It should come as fast as possible.
New major should be later with removing of old stuff

@JohnAlbin
Copy link
Collaborator

JohnAlbin commented Oct 17, 2023

So microbundle doesn't handle entry points in subdirectories because the output module file is placed in the root of the output directory instead of into a subdirectory of the output directory. We can work around this.

Also, I found this issue with microbundle related to TypeScript. We can work around that as well by adding a post-build step to copy .d.ts files.

do we need to add App Router support to codebases using next-drupal 1.0’s get[DrupalThing] functions?

I think, yes. I've created a /query bundle for those functions.

My draft PR includes these 6 new entry points:

import * from "next-drupal"; // All the exports, same as 1.6.0 release.
import * from "next-drupal/client";
import * from "next-drupal/navigation"; // The only one that is not Server Component compatible.
import * form "next-drupal/preview";
import * from "next-drupal/query"; // Contains get[DrupalThing] functions.
import * form "next-drupal/translation";
import * from "next-drupal/utils";

@JohnAlbin
Copy link
Collaborator

Ok. An experimental build with the new entry points is now available on npmjs.com.

#559 (comment)

🎉 Experimental release published 📦️ on npm!

pnpm add next-drupal@0.0.0-pr.559.0c3c8cef
yarn add next-drupal@0.0.0-pr.559.0c3c8cef
npm i next-drupal@0.0.0-pr.559.0c3c8cef

I'm going to start my own testing in a project, but it would be great if others try it out too. The more testing now the sooner we'll have a real release.

Also, keep the comments about this solution coming. They are most welcome!

@JohnAlbin
Copy link
Collaborator

We had some internal discussions and decided we need to map out the upgrade path properly before deciding on the actual solution.

Right now, code using Next Drupal fall into 3 categories:

  1. Using Next.js' pages/, environment variables like NEXT_PUBLIC_DRUPAL_BASE_URL, and getResource*() functions.
  2. Using Next.js' pages/ with useMenu() (and environment variables)
  3. Using Next.js' pages/ and new DrupalClient().

Everything is client-side code so a single entry point is fine.

Configuring next-drupal with environment variables is deprecated (#553) so those codebases will need to upgrade to newer APIs. But we shouldn't require the upgrade to App Router at the same time. Those 2 different upgrades should be independent of each other to give developers flexibility.

Since Next.js supports both App Router and pages/ router to facilitate migrating code gradually to the newer APIs, Next Drupal will need to do the same until Next.js drops pages/ support.

So the next major version of Next Drupal will need to support all of these scenarios at the same time:

  1. Using pages/ with getResource*()
  2. Using pages/ using DrupalClient()
  3. Using App Router with getResource*()
  4. Using App Router with DrupalClient()
  5. Using useMenu() in Client Components

Scenario 5 means useMenu needs to be available in its own entry point, import { useMenu } from "next-drupal/navigation". (This entry point name mirrors Next.js' own next/navigation entry point for useRouter() etc.)

Scenarios 3 and 4 mean the entry points for all the other current functions and classes need to be available in an entry point that does NOT contain Client-Component-only code.

In a previous comment I said:

So there are two paths to fixing that:

  1. Breaking change; release 2.0.0. Copy all 1.x exports to a new entry point (maybe import {} from 'next-drupal/deprecated'?)

Actually, I was wrong. All we really need to do move useMenu into its own entry point. And tell people that use that function they need to change any import { useMenu } from "next-drupal" to import { useMenu } from "next-drupal/navigation". That's a pretty small, easy-to-implement change for developers.

The other path to fixing this issue is what is in the experimental release above. No breaking change now. Anyone creating App Router pages must use one of the new entry points. Then when 2.x, we can remove whatever exports from the default entry that we want. Which means the upgrade would be very similar to just releasing 2.x now. Which… sounds like path 1 with more steps?

I'll create a new experimental release later this week.

@JohnAlbin JohnAlbin added this to the next-drupal 2.0.0 milestone Oct 20, 2023
@Danishkhurshid
Copy link

@JohnAlbin Separating breaking changes related to the App and Pages directories into their dedicated entry points, aligned with NextJs naming conventions, is a better approach. This would ensure that the pages directory, which is not set to be deprecated, can still be supported effectively. Great work 🙌.

JohnAlbin added a commit that referenced this issue Oct 23, 2023
Fixes #442

BREAKING CHANGE:

The useMenu() client hook has moved out of the main entry point and into its own
entry point. Any import or require of that function needs to be updated:

Old usage:
```js
import { useMenu } from "next-drupal"
```

New usage:
```js
import { useMenu } from "next-drupal/navigation"
```
JohnAlbin added a commit that referenced this issue Nov 1, 2023
Fixes #442

BREAKING CHANGE:

The useMenu() client hook has moved out of the main entry point and into its own
entry point. Any import or require of that function needs to be updated:

Old usage:
```js
import { useMenu } from "next-drupal"
```

New usage:
```js
import { useMenu } from "next-drupal/navigation"
```
JohnAlbin added a commit that referenced this issue Nov 15, 2023
Fixes #442

BREAKING CHANGE:

The useMenu() client hook has moved out of the main entry point and into its own
entry point. Any import or require of that function needs to be updated:

Old usage:
```js
import { useMenu } from "next-drupal"
```

New usage:
```js
import { useMenu } from "next-drupal/navigation"
```
JohnAlbin added a commit that referenced this issue Nov 17, 2023
Fixes #442

BREAKING CHANGE:

The useMenu() client hook has moved out of the main entry point and into its own
entry point. Any import or require of that function needs to be updated:

Old usage:
```js
import { useMenu } from "next-drupal"
```

New usage:
```js
import { useMenu } from "next-drupal/navigation"
```
JohnAlbin added a commit that referenced this issue Nov 17, 2023
Fixes #442

BREAKING CHANGE:

The useMenu() client hook has moved out of the main entry point and into its own
entry point. Any import or require of that function needs to be updated:

Old usage:
```js
import { useMenu } from "next-drupal"
```

New usage:
```js
import { useMenu } from "next-drupal/navigation"
```
JohnAlbin added a commit that referenced this issue Nov 17, 2023
Fixes #442

BREAKING CHANGE:

The useMenu() client hook has moved out of the main entry point and into its own
entry point. Any import or require of that function needs to be updated:

Old usage:
```js
import { useMenu } from "next-drupal"
```

New usage:
```js
import { useMenu } from "next-drupal/navigation"
```
@JohnAlbin
Copy link
Collaborator

Sorry for the delay in getting back to this issue; I've had some health issues (kidney stone).

But I've made 2 new experimental releases that support the App Router with Next.js v14 (using 2 different bundlers). We're doing some internal testing and aren't sure which bundler we will go with, but you can test out the releases at #600 or #583

I'm working on upgrading the basic-starter to use the App Router now (to help with our testing).

JohnAlbin added a commit that referenced this issue Nov 19, 2023
Fixes #442

BREAKING CHANGE:

The useMenu() client hook has moved out of the main entry point and into its own
entry point. Any import or require of that function needs to be updated:

Old usage:
```js
import { useMenu } from "next-drupal"
```

New usage:
```js
import { useMenu } from "next-drupal/navigation"
```
JohnAlbin added a commit that referenced this issue Nov 19, 2023
Fixes #442

BREAKING CHANGE:

The useMenu() client hook has moved out of the main entry point and into its own
entry point. Any import or require of that function needs to be updated:

Old usage:
```js
import { useMenu } from "next-drupal"
```

New usage:
```js
import { useMenu } from "next-drupal/navigation"
```
JohnAlbin added a commit that referenced this issue Nov 20, 2023
Fixes #442

BREAKING CHANGE:

The useMenu() client hook has moved out of the main entry point and into its own
entry point. Any import or require of that function needs to be updated:

Old usage:
```js
import { useMenu } from "next-drupal"
```

New usage:
```js
import { useMenu } from "next-drupal/navigation"
```
JohnAlbin added a commit that referenced this issue Nov 22, 2023
Fixes #442

BREAKING CHANGE:

The useMenu() client hook has moved out of the main entry point and into its own
entry point. Any import or require of that function needs to be updated:

Old usage:
```js
import { useMenu } from "next-drupal"
```

New usage:
```js
import { useMenu } from "next-drupal/navigation"
```
JohnAlbin added a commit that referenced this issue Nov 22, 2023
Fixes #442

BREAKING CHANGE:

The useMenu() client hook has moved out of the main entry point and into its own
entry point. Any import or require of that function needs to be updated:

Old usage:
```js
import { useMenu } from "next-drupal"
```

New usage:
```js
import { useMenu } from "next-drupal/navigation"
```
@JohnAlbin
Copy link
Collaborator

We've decided to go with the tsup bundler. As soon as #600 is merged, you can track other Next.js v14 related issues in the 2.0.0 milestone. https://github.com/chapter-three/next-drupal/milestone/9

@Danishkhurshid
Copy link

@JohnAlbin Hope your health improves rapidly and thank you for the update. tsup is what I go for too 👍 preconstruct would also have been great choice.

@paolomainardi
Copy link

@JohnAlbin my best wishes for a speedy recovery too. I have just followed this discussion and now I have the overall issue much more clear, anyway I am still struggling to understand how to use internal APIs which expect the context object and how it should be treated in the App Router implementations.

Added here some details: #601 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: next-drupal enhancement New feature or request
Projects
None yet
Development

No branches or pull requests