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

Generate SSG Page used as cache for user's third-party calendar #6775

Merged
merged 79 commits into from Feb 13, 2023

Conversation

roae
Copy link
Contributor

@roae roae commented Jan 29, 2023

What does this PR do?

  • Generate a SSG page used as cache for user`s third-party calendars by month, this page revalidates each second.
  • tRCP getSchedule endpoint change: now get the generated json file for the SSG page which is normally used by NextJS in the browser navigation to get the third-party calendar data

TODO

  • Revalidate cache page when users connect/disconnect a calendar.
  • Revalidate when users change the selected calendars.
  • Revalidate when the user adds or deletes an event directly in the connected calendars via webhooks.
  • Add API endpoint to revalidates several months ahead.

Fixes #4535
/claim #4535

Note: On development environment the response time of getSchedule endpoint increases because NextJS compiles the cache page first. To notice the improvement you must run a production build.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Chore (refactoring code, technical debt, workflow improvements)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist

  • I haven't read the contributing guide
  • My code doesn't follow the style guidelines of this project
  • I haven't checked if my PR needs changes to the documentation
  • I haven't checked if my changes generate no new warnings
  • I haven't added tests that prove my fix is effective or that my feature works
  • I haven't checked if new and existing unit tests pass locally with my changes

@vercel
Copy link

vercel bot commented Jan 29, 2023

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

Name Status Preview Comments Updated
cal ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 13, 2023 at 6:44PM (UTC)
1 Ignored Deployment
Name Status Preview Comments Updated
ui ⬜️ Ignored (Inspect) Visit Preview Feb 13, 2023 at 6:44PM (UTC)

@vercel
Copy link

vercel bot commented Jan 29, 2023

@roae is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

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.

Very clever approach. cc @emrysal can you take a look given a chance?

apps/web/package.json Outdated Show resolved Hide resolved
packages/core/CalendarManager.ts Show resolved Hide resolved
@PeerRich
Copy link
Member

PeerRich commented Feb 2, 2023

is this related to the RFC for caching calendars or is this unrelated?

@zomars
Copy link
Member

zomars commented Feb 2, 2023

is this related to the RFC for caching calendars or is this unrelated?

It is a different approach @PeerRich no need for a custom caldav server (at least not yet)

@PeerRich
Copy link
Member

PeerRich commented Feb 2, 2023

@roae can you uncommit your yarn.lock? (take it from our main)

@roae
Copy link
Contributor Author

roae commented Feb 2, 2023

@roae can you uncommit your yarn.lock? (take it from our main)

Done!

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.

Nitpicks. Will test properly on Monday

apps/web/pages/[user]/calendar-cache/[month].tsx Outdated Show resolved Hide resolved
apps/web/pages/[user]/calendar-cache/[month].tsx Outdated Show resolved Hide resolved
packages/core/CalendarManager.ts Outdated Show resolved Hide resolved
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.

LFG!

Ship it

@PeerRich
Copy link
Member

HOLY SHIT 4s down to 240ms?!

@zomars
Copy link
Member

zomars commented Feb 10, 2023

HOLY SHIT 4s down to 240ms?!

Of course it varies between calls but still pretty impressive 🙌🏽

Copy link
Member

@PeerRich PeerRich left a comment

Choose a reason for hiding this comment

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

unfortunately there is a new skeleton loader that wasnt there on current prod

CleanShot.2023-02-10.at.22.28.19.mp4

@zomars
Copy link
Member

zomars commented Feb 10, 2023

unfortunately there is a new skeleton loader that wasnt there on current prod

CleanShot.2023-02-10.at.22.28.19.mp4

Adding more context to this change. We discovered that if a slot got taken and refetched it didn't disappear in the UI due the use of our local state. But agreed that we can improve the UX to match the previous one.

@t3dotgg
Copy link

t3dotgg commented Feb 11, 2023

I'm like 70% sure you can return from an API endpoint in Next with infinite cache times and not have to do the empty page hack - I do this all the time - revalidate works on API endpoints

@zomars zomars requested a review from PeerRich February 13, 2023 18:10
@zomars
Copy link
Member

zomars commented Feb 13, 2023

I'm like 70% sure you can return from an API endpoint in Next with infinite cache times and not have to do the empty page hack - I do this all the time - revalidate works on API endpoints

Care to elaborate @t3dotgg ? Is this cache useful for every user or would it only work per browser/request?

@zomars
Copy link
Member

zomars commented Feb 13, 2023

unfortunately there is a new skeleton loader that wasnt there on current prod

CleanShot.2023-02-10.at.22.28.19.mp4

Skeleton got fixed

https://www.loom.com/share/4351b946528349dd87e201aa98bbca32

@zomars
Copy link
Member

zomars commented Feb 13, 2023

Merging for now. We can always follow up with further improvements. Talked to @roae and it seems like the next big thing to solve would be reducing bundle size.

image

@PeerRich
Copy link
Member

nice lets merge! should I or do you wanna click the magic green button

@PeerRich PeerRich closed this Feb 13, 2023
@zomars zomars merged commit afb9605 into calcom:main Feb 13, 2023
@PeerRich
Copy link
Member

whopsy wrong button clicky

@zomars
Copy link
Member

zomars commented Feb 13, 2023

This Is Fine GIF

@leventdeniz
Copy link
Contributor

leventdeniz commented Feb 21, 2023

Talked to @roae and it seems like the next big thing to solve would be reducing bundle size.

I searched the current main. There are a few cases where the entire lodash is being imported for no reason. Should be a very easy fix that instantly reduces bundle size.

image

Did this in another project the other day, before/after:
image image

@PeerRich
Copy link
Member

i think we can get rid of lodash alltogether no? most of lodash is ECMA specs now anyway

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
❗️ .env changes contains changes to env variables ❗️ migrations contains migration files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CAL-119] Initial rendering of calendar takes 3-5 seconds!