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

removed SSR trpc calls instead SPA for logged in pages #6936

Merged
merged 17 commits into from
Feb 24, 2023

Conversation

PeerRich
Copy link
Member

@PeerRich PeerRich commented Feb 8, 2023

fixes #6935

so far so good on localhost

Why SSR in the first place:

we used to have i18n flickering with CSR but i cant reproduce them anymore. ideally we dont rely on SSR for logged in pages, since its slower

what to test

  • hard reload some pages
  • see if i18n keys are flickering
  • change language in settings

@PeerRich PeerRich linked an issue Feb 8, 2023 that may be closed by this pull request
@linear
Copy link

linear bot commented Feb 8, 2023

CAL-1006 Experiment: try dashboard as SPA vs SSR

Posted by @KATT

Heya 👋 come the [suspense](http://app.cal.com|app.cal.com> dashboard is server-side rendered? I feel it would feel faster if you did SPA there and you'd ship faster since it would unlock being able to use <https://trpc.io/docs/suspense) etc

Below is a almost 3s blocking request when toggling between "Availability" and "Booking"

Like, my suggestion would be to get rid of this stuff:

export const getServerSideProps = async (context: GetServerSidePropsContext) => {
const ssr = await ssrInit(context);
return {
props: {
trpcState: ssr.dehydrate(),
},
};
};

IIRC that stuff is there because of i18n-flicker, but I'm sure there's an alternative way of dealing with that

Suspense (Experimental) | tRPC

<https://github.com/calcom/cal.com/blob/8c150264f498b8e5d5cfedaebc69465c1dfd2aeb/apps/web/pages/availability/index.tsx | index.tsx>

@vercel
Copy link

vercel bot commented Feb 8, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
cal ❌ Failed (Inspect) Feb 24, 2023 at 1:50AM (UTC)
1 Ignored Deployment
Name Status Preview Comments Updated
ui ⬜️ Ignored (Inspect) Visit Preview Feb 24, 2023 at 1:50AM (UTC)

@AaronPresley
Copy link
Contributor

The main issue I'm seeing on this is that the page no longer updates the UI strings when changing your preferred language under Settings > General. This felt surprising to me, as I double-checked this workflow as part of my previous PR.

I checked out main and am seeing the same problem, so it's not necessarily related to this PR directly. I see there have been several commits relating to header cache since my PR. Might be worth spinning up an issue?

Copy link
Contributor

@AaronPresley AaronPresley left a comment

Choose a reason for hiding this comment

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

Looking good overall, despite what I mentioned in this comment which should likely be a new Issue. Otherwise I don't see any flickering.

@PeerRich
Copy link
Member Author

Might be worth spinning up an issue?

oh yes please! seems like a regression

@PeerRich PeerRich requested a review from zomars February 14, 2023 10:11
@PeerRich
Copy link
Member Author

hmm checks failing, and tests. not sure why. maybe @zomars knows more

@zomars
Copy link
Member

zomars commented Feb 14, 2023

Type checks are failing:

image

image

You should be able to click on Details and see that we're trying to use a now removed getServerSideProps in a page

Copy link
Member

@zomars zomars left a comment

Choose a reason for hiding this comment

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

Blocking to type fix, also this PR is leaving a lot of unused imports

@github-actions
Copy link
Contributor

github-actions bot commented Feb 20, 2023

📦 Next.js Bundle Analysis

This analysis was generated by the next.js bundle analysis action 🤖

This PR introduced no changes to the javascript bundle 🙌

@zomars
Copy link
Member

zomars commented Feb 20, 2023

This breaking Embed tests. cc @hariombalhara to help debugging

@@ -128,17 +125,11 @@ const Item = ({ type, group, readOnly }: { type: EventType; group: EventTypeGrou
data-testid={"event-type-title-" + type.id}>
{type.title}
</span>
{group.profile.slug ? (
Copy link
Member Author

Choose a reason for hiding this comment

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

this is not from me :o

Copy link
Member Author

Choose a reason for hiding this comment

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

before we merge can we undo this? i dont know where this came from @zomars @roae

{readOnly && (
<span className="inline items-center rounded-sm bg-gray-100 px-1.5 py-0.5 text-xs font-medium text-gray-800 ltr:ml-2 ltr:mr-2 rtl:ml-2">
<span className="rtl:ml-2inline items-center rounded-sm bg-gray-100 px-1.5 py-0.5 text-xs font-medium text-gray-800 ltr:ml-2 ltr:mr-2">
Copy link
Member Author

Choose a reason for hiding this comment

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

same here. also typo

Suggested change
<span className="rtl:ml-2inline items-center rounded-sm bg-gray-100 px-1.5 py-0.5 text-xs font-medium text-gray-800 ltr:ml-2 ltr:mr-2">
<span className="rtl:ml-2 inline items-center rounded-sm bg-gray-100 px-1.5 py-0.5 text-xs font-medium text-gray-800 ltr:ml-2 ltr:mr-2">

Copy link
Contributor

Choose a reason for hiding this comment

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

the span tag is already inline, it is redundant

Comment on lines 566 to 578
const { t } = useLocale();
const router = useRouter();

const publishTeamMutation = trpc.viewer.teams.publish.useMutation({
onSuccess(data) {
router.push(data.url);
},
onError: (error) => {
showToast(error.message, "error");
},
});

Copy link
Member Author

Choose a reason for hiding this comment

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

also not by me

Copy link
Member Author

Choose a reason for hiding this comment

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

@zomars
Copy link
Member

zomars commented Feb 21, 2023

Can you take over this PR @roae

…a-vs-ssr

# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
Copy link
Contributor

@roae roae left a comment

Choose a reason for hiding this comment

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

Looking good overall, I never saw any flickering, even changing the language in settings

@roae roae requested a review from zomars February 24, 2023 01:41
@zomars zomars merged commit 9e08cf8 into main Feb 24, 2023
@zomars zomars deleted the 6935-cal-1006-experiment-try-dashboard-as-spa-vs-ssr branch February 24, 2023 01:41
fritterhoff pushed a commit to hm-edu/cal.com that referenced this pull request Feb 26, 2023
* removed SSR trpc

* Re added debug endpoint

* Update me.ts

* Fixes unused imports

* Rollback unrelated changes

---------

Co-authored-by: zomars <zomars@me.com>
Co-authored-by: Efraín Rochín <roae.85@gmail.com>
Mythie pushed a commit to Mythie/cal.com that referenced this pull request Mar 5, 2023
* removed SSR trpc

* Re added debug endpoint

* Update me.ts

* Fixes unused imports

* Rollback unrelated changes

---------

Co-authored-by: zomars <zomars@me.com>
Co-authored-by: Efraín Rochín <roae.85@gmail.com>
dmkav pushed a commit to join-com/cal-com that referenced this pull request Mar 6, 2023
* removed SSR trpc

* Re added debug endpoint

* Update me.ts

* Fixes unused imports

* Rollback unrelated changes

---------

Co-authored-by: zomars <zomars@me.com>
Co-authored-by: Efraín Rochín <roae.85@gmail.com>
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.

[CAL-1006] Experiment: try dashboard as SPA vs SSR
4 participants