search

keystonejs / keystone

The superpowered headless CMS for Node.js — built with GraphQL and React
https://keystonejs.com
MIT License
9.18k stars 1.15k forks source link

Fix `@keystone-6/fields-document` package breaking when compiling in SSR environments #9041

Closed marekryb closed 4 months ago

marekryb commented 7 months ago

This attempts to fix https://github.com/keystonejs/keystone/issues/8717 (Attempted to call validateAndNormalizeDocument() from the server).

As stated in https://github.com/keystonejs/keystone/pull/8403#issuecomment-1478851798 that made a workaround by adding use client clauses:

The root cause of this is the crossover of React/Next code

In principle, this PR splits bunch of .tsx files into two separate ones. One for react-related (client-side) code and second one for platform-agnostic code. I am keeping the original file for backwards compatibility. For example file: packages/fields-document/src/DocumentEditor/insert-menu.tsx has been split into: packages/fields-document/src/DocumentEditor/insert-menu-model.ts packages/fields-document/src/DocumentEditor/insert-menu-view.tsx and the original file is now exporting above two:

export * from './insert-menu-model'
export * from './insert-menu-view'

External exports do not change, however internally, files that take part in validation process now import -model.ts files that are not importing disallowed hooks (useState etc).

There are no functional changes in the code apart from one in packages/fields-document/src/DocumentEditor/utils-model.ts::insertNodesButReplaceIfSelectionIsAtEmptyParagraphOrHeading. I will write about this one in the comment below.

It seems to work fine with my limited testing and all unit tests are passing. However I did not test this in some more advanced scenario and with custom components yet.

Please let me know whether this is something that you are willing to work on and eventually accept.

codesandbox-ci[bot] commented 7 months ago

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 3de0ecf3aaca61fec030f4dee7d265deba4dad7d:

Sandbox Source
@keystone-6/sandbox Configuration
dcousens commented 7 months ago

I am really open to accepting this work, my only concern is that we're trying to solve a number of problems in one pull request. If we can break this into a few smaller pull requests, I can review and approve more quickly.

dcousens commented 7 months ago

Can you please put any code moving operations into their own commits, so I can easily review those commits and then focus only on the diff?

marekryb commented 7 months ago

I am really open to accepting this work, my only concern is that we're trying to solve a number of problems in one pull request. If we can break this into a few smaller pull requests, I can review and approve more quickly.

Unfortunately this is not possible. Everything here is related and do not function without other pieces.

EDIT: The best I can do is to move changes in examples/framework-nextjs-app-directory to separate PR.

Can you please put any code moving operations into their own commits, so I can easily review those commits and then focus only on the diff?

This is already kind of the case. The initial ~18 "wip" labelled commits are splitting one file and then changing related imports in other places.

My workflow was like this: 1) remove existing 'use client' to trigger error 2) see the error when running examples/framework-nextjs-app-directory 3) split the file that has the error and fix imports related to it 4) run pnpm jest packages/fields-document/ 5) git commit -am "wip" 6) goto 2 until working

marekryb commented 7 months ago

Removed all changes to examples/framework-nextjs-app-directory. Can be split to separate PR when this is done.

dcousens commented 7 months ago

@marekryb OK, if you can get everything into a working order, as atomic as reasonable, I could jump in towards the end

dcousens commented 5 months ago

I have updated this pull request and rebased each of the changes. I effectively did the changes again from HEAD, to ensure I understood everything that was changed, but the credit and work will still be attributed to you @marekryb when merged.

Thanks again for this @marekryb, and I have the same conclusion, this may not resolve every situation for now, but, we should be nearer to that with this pull request.

marekryb commented 4 months ago

Thank you for pushing this forward @dcousens.

I made this PR when trying to upgrade some older project to nextjs with RSC, but priorities changed etc... Sorry for no updates.

This should solve the problem when using stock editor (yay). However last time I tried, it still breaks when using custom components. As I remember my conclusion at that time was that the interface for custom components needs to be changed to make it work as well. Something to look at later time perhaps.