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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 22 commits into from May 23, 2024
Merged

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

merged 22 commits into from May 23, 2024

Conversation

marekryb
Copy link
Contributor

@marekryb marekryb commented Feb 25, 2024 •
edited

This attempts to fix #8717 (Attempted to call validateAndNormalizeDocument() from the server).

As stated in #8403 (comment) 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 CodeSandbox CI
Copy link

codesandbox-ci bot commented Feb 25, 2024 •
edited

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 3de0ecf:

Sandbox Source
@keystone-6/sandbox Configuration

@dcousens
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

marekryb commented Feb 26, 2024 •
edited

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
Copy link
Contributor Author

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

@dcousens
Copy link
Member

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

@dcousens dcousens marked this pull request as ready for review May 8, 2024 01:19
@dcousens dcousens changed the title WIP/POC Refactor fields-document package to fix server-side validation Update fields-document package to fix server-side validation May 8, 2024
@dcousens
Copy link
Member

dcousens commented May 8, 2024

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 dcousens force-pushed the editor branch 4 times, most recently from c7a88fb to f96e4c4 Compare May 14, 2024 05:24
@dcousens dcousens added dependencies Related to our dependencies 🐛 bug Unresolved bug labels May 23, 2024
@dcousens dcousens self-assigned this May 23, 2024
@dcousens dcousens changed the title Update fields-document package to fix server-side validation Fix @keystone-6/fields-document package breaking when compiling in SSR environments May 23, 2024
@dcousens dcousens merged commit 7197750 into keystonejs:main May 23, 2024
43 checks passed
@marekryb
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcousens dcousens dcousens left review comments

Assignees

@dcousens dcousens

Labels
🐛 bug Unresolved bug dependencies Related to our dependencies
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Attempted to call validateAndNormalizeDocument() from the server [fields-document]
2 participants
@marekryb @dcousens