keystone icon indicating copy to clipboard operation
keystone copied to clipboard

Update `fields-document` package to fix server-side validation

Open marekryb opened this issue 1 year ago • 7 comments

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.

marekryb avatar Feb 25 '24 14:02 marekryb

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

codesandbox-ci[bot] avatar Feb 25 '24 14:02 codesandbox-ci[bot]

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 avatar Feb 26 '24 01:02 dcousens

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?

dcousens avatar Feb 26 '24 01:02 dcousens

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 avatar Feb 26 '24 01:02 marekryb

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

marekryb avatar Feb 26 '24 01:02 marekryb

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

dcousens avatar Feb 26 '24 03:02 dcousens

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.

dcousens avatar May 08 '24 01:05 dcousens

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.

marekryb avatar May 23 '24 13:05 marekryb