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

Control panel list SSR #3749

Merged
merged 19 commits into from
May 11, 2023
Merged

Control panel list SSR #3749

merged 19 commits into from
May 11, 2023

Conversation

JeffersonBledsoe
Copy link
Member

@JeffersonBledsoe JeffersonBledsoe commented Oct 11, 2022

Remove client-side fetching of control panels and perform it on the server. Prevents flash of unfinished control panel list

@netlify
Copy link

netlify bot commented Oct 11, 2022

Deploy Preview for volto canceled.

Name Link
🔨 Latest commit da8096a
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/645cc19bdf2df1000883540f

@cypress
Copy link

cypress bot commented Oct 11, 2022

Passing run #5098 ↗︎

0 493 20 0 Flakiness 0

Details:

Merge branch 'master' into control-panel-ssr
Project: Volto Commit: da8096a6a3
Status: Passed Duration: 12:34 💡
Started: May 11, 2023 10:24 AM Ended: May 11, 2023 10:37 AM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@JeffersonBledsoe
Copy link
Member Author

@sneridagh I'll look into the failing unit test shortly. Any reason the control panel list wasn't already using asyncConnect to fetch the list? I've noticed this in a few places too and would like to update those

Copy link
Contributor

@tiberiuichim tiberiuichim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome that you're converting from class to function.

@JeffersonBledsoe
Copy link
Member Author

JeffersonBledsoe commented Oct 12, 2022

Awesome that you're converting from class to function.

@tiberiuichim Thanks! If there was the interest, I'd definitely be up for converting more components to functional components. It's alot to learn both class components and functional for developers not familiar with React.

As an aside, any idea what's up with the tests? I'm not sure how to handle async in jest while still letting it render correctly in Cypress

@JeffersonBledsoe
Copy link
Member Author

JeffersonBledsoe commented Oct 13, 2022

@tiberiuichim Should the client still do a fetch to get the control panels, even though they should all be there from the SSR asyncConnect call? Seems wasteful to me

@tiberiuichim
Copy link
Contributor

@JeffersonBledsoe This is one of the things that (IMHO) we do wrong with the current approach in Volto. See #2751 for some context.

The issue is a bit historic, to say so: in principle, as far as I understand, AsyncConnect, as a wrapper, would be able to inject async data as props in both SSR and Client. That's why when I've implemented the configuration-based extension mechanism for AsyncConnect, I've named them "asyncPropsExtenders", with an accent on "asyncProps". As far as I can tell, we should not be triggering any async network fetch action from inside the component - exactly your question, (as far as possible, as far as it makes sense to do so), and instead of just relying on the props being injected by the AsyncConnect. The way to go is to remove the __SERVER__ condition in App.jsx, but I guess we didn't want to do it because of time constraints to check that everything works.

@tiberiuichim
Copy link
Contributor

I'm not sure how to handle async in jest while still letting it render correctly in Cypress

See https://training.plone.org/5/effective-volto/testing/unit.html

@erral
Copy link
Member

erral commented Oct 13, 2022

Perhaps this is a stupid question, but why do we need SSR for control panels? This is never a public page and there is no need to render that on the server, right?

@JeffersonBledsoe
Copy link
Member Author

Perhaps this is a stupid question, but why do we need SSR for control panels? This is never a public page and there is no need to render that on the server, right?

Speed and the flash of an incomplete control panel list are the reasons I started this PR. I occasionally work on servers that aren't in my country and there's a noticeable delay between seeing the initial control panel page and having to wait to see the control panel I want to open

@JeffersonBledsoe JeffersonBledsoe marked this pull request as ready for review March 16, 2023 03:26
@sneridagh sneridagh merged commit db06655 into master May 11, 2023
@sneridagh sneridagh deleted the control-panel-ssr branch May 11, 2023 10:39
sneridagh added a commit to ninanoleto/volto that referenced this pull request May 20, 2023
* master: (29 commits)
  Remove max_line_length from .editorconfig (plone#4776)
  Fix bug showing logs at the browsers when richtext widget is use (plone#4780)
  Show expired and future content in contents view (plone#4764)
  Fix reducing expanders loaded in a subrequest (plone#4761)
  Update release notes for 16.20.5 and 16.20.6 (plone#4759)
  Release 17.0.0-alpha.7
  Try to sort out volto's use of language codes (plone#4741)
  Improve .npmignore to not include not needed files/folders (plone#4746)
  Release 17.0.0-alpha.6
  Control panel list SSR (plone#3749)
  Open all accordion'd content in InlineForm by default, allow arbitrarily close any number of them (plone#4178)
  Upgrade to Plone 6.0.4 (plone#4743)
  fix: unresponsive add page (plone#4507)
  Apply suggestion from browser for password field (plone#4524)
  (fix):Object.normaliseMail: Cannot read properties of null (plone#4558)
  Fix link in Volto, remove from linkcheck ignore in Documentation v6.0 (plone#4742)
  added documentation regarding the static middleware plone#4518 (plone#4736)
  Closes issue plone#4567 (plone#4570)
  Fix whitespace in locales created by the generator (plone#4737)
  Tidy up from synch with 16.x.x (plone#4728)
  ...
sneridagh added a commit that referenced this pull request May 20, 2023
* master: (29 commits)
  Remove max_line_length from .editorconfig (#4776)
  Fix bug showing logs at the browsers when richtext widget is use (#4780)
  Show expired and future content in contents view (#4764)
  Fix reducing expanders loaded in a subrequest (#4761)
  Update release notes for 16.20.5 and 16.20.6 (#4759)
  Release 17.0.0-alpha.7
  Try to sort out volto's use of language codes (#4741)
  Improve .npmignore to not include not needed files/folders (#4746)
  Release 17.0.0-alpha.6
  Control panel list SSR (#3749)
  Open all accordion'd content in InlineForm by default, allow arbitrarily close any number of them (#4178)
  Upgrade to Plone 6.0.4 (#4743)
  fix: unresponsive add page (#4507)
  Apply suggestion from browser for password field (#4524)
  (fix):Object.normaliseMail: Cannot read properties of null (#4558)
  Fix link in Volto, remove from linkcheck ignore in Documentation v6.0 (#4742)
  added documentation regarding the static middleware #4518 (#4736)
  Closes issue #4567 (#4570)
  Fix whitespace in locales created by the generator (#4737)
  Tidy up from synch with 16.x.x (#4728)
  ...
sneridagh added a commit to ninanoleto/volto that referenced this pull request May 20, 2023
* typescript-support-in-core: (29 commits)
  Remove max_line_length from .editorconfig (plone#4776)
  Fix bug showing logs at the browsers when richtext widget is use (plone#4780)
  Show expired and future content in contents view (plone#4764)
  Fix reducing expanders loaded in a subrequest (plone#4761)
  Update release notes for 16.20.5 and 16.20.6 (plone#4759)
  Release 17.0.0-alpha.7
  Try to sort out volto's use of language codes (plone#4741)
  Improve .npmignore to not include not needed files/folders (plone#4746)
  Release 17.0.0-alpha.6
  Control panel list SSR (plone#3749)
  Open all accordion'd content in InlineForm by default, allow arbitrarily close any number of them (plone#4178)
  Upgrade to Plone 6.0.4 (plone#4743)
  fix: unresponsive add page (plone#4507)
  Apply suggestion from browser for password field (plone#4524)
  (fix):Object.normaliseMail: Cannot read properties of null (plone#4558)
  Fix link in Volto, remove from linkcheck ignore in Documentation v6.0 (plone#4742)
  added documentation regarding the static middleware plone#4518 (plone#4736)
  Closes issue plone#4567 (plone#4570)
  Fix whitespace in locales created by the generator (plone#4737)
  Tidy up from synch with 16.x.x (plone#4728)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants