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

Static build #220

Merged
merged 22 commits into from
Dec 19, 2024
Merged

Static build #220

merged 22 commits into from
Dec 19, 2024

Conversation

mimiflynn
Copy link
Member

As a first step for #177 this PR converts the Vite React AJAX json call to a static build of the calendar with the data already included and rendered on the server using Gatsby.

There are no visual or functional changes to the calendar; this update only impacts build and output.

@mimiflynn mimiflynn added the enhancement New feature or request label Dec 10, 2024
@mimiflynn mimiflynn self-assigned this Dec 10, 2024
useState,
} from 'react';

import events from '../../dist/events.json';
Copy link
Member Author

Choose a reason for hiding this comment

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

Pulling the event.json at time of build, so updated the PR build.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You think we could do some pre-culling here, and only display events for the last-next 12 months (and don't allow the calendar to be forwarded over that 12 months), to cut back the size of the build/download and make it more performant. Definitely not part of this PR, rather as a future issue to be solved.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Interestingly, the performance boost is so big already from the static change alone, that we do not necessarily need my suggested change at all.

@mimiflynn mimiflynn marked this pull request as ready for review December 11, 2024 16:23
@mimiflynn mimiflynn requested review from TheJuanAndOnly99, maoo, robmoffat and psmulovics and removed request for TheJuanAndOnly99 December 11, 2024 16:23
.eslintrc.json Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

Gatsby includes eslint with a comprehensive config.

@@ -21,8 +21,10 @@ jobs:
with:
node-version: '${{ matrix.node-version }}'

- name: Get events mock for PR build
run: npm run get-mock-events

Copy link
Member Author

Choose a reason for hiding this comment

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

Run PR build with mock events to ensure the build is successful.

mock/events.json Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

Mock events to be used for PR build to test that server side rendering will build successfully.

Copy link
Member Author

Choose a reason for hiding this comment

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

Script to copy the mock events.json into /dist to mimic the google calendar event.json pull.

setShowEventDetails(false);
window.outerWidth < 600 && setPopupPosition({ left: 0, top: 0 });
!isMinWidth() && setPopupPosition({ left: 0, top: 0 });
Copy link
Member Author

Choose a reason for hiding this comment

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

Since the build creates a server side rendering of the site, window is not available and fails the build.

@@ -202,7 +198,7 @@ function Calendar() {
aspectRatio={aspectRatio}
handleWindowResize={true}
windowResize={windowResize}
events="events.json"
events={events}
Copy link
Member Author

Choose a reason for hiding this comment

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

Applying the events that were pulled as events.json at the top of this file.

@psmulovics
Copy link
Collaborator

Wow, this hopefully will speed up the load of the page significantly.

@robmoffat
Copy link
Member

this sounds amazing. is there a preview anywhere?

@psmulovics
Copy link
Collaborator

this sounds amazing. is there a preview anywhere?

@robmoffat , we would need @maoo 's help on setting up a similar staging site on netlify like we have for the others; so, not yet. If you could note to him to do it, that would help :)

@robmoffat
Copy link
Member

this sounds amazing. is there a preview anywhere?

@robmoffat , we would need @maoo 's help on setting up a similar staging site on netlify like we have for the others; so, not yet. If you could note to him to do it, that would help :)

Ok, having a crack at this myself...

@robmoffat
Copy link
Member

Ok, previews now working! (for Gatsby at least)

See: https://deploy-preview-221--calendar-gatsby.netlify.app
Which is based on my PR: #221

You'll have to incorporate my changes into your PR to get it to preview properly, @mimiflynn

Copy link

netlify bot commented Dec 12, 2024

Deploy Preview for calendar-gatsby failed. Why did it fail? →

Built without sensitive environment variables

Name Link
🔨 Latest commit 997e775
🔍 Latest deploy log https://app.netlify.com/sites/calendar-gatsby/deploys/6762f2a5bf9732000738dca5

@mimiflynn
Copy link
Member Author

@robmoffat I pulled your branch into this one and created a new script for PR builds that don't have access to secrets. Please see the comment above for deploy request approval.

@robmoffat
Copy link
Member

Ok, this build script doesn't work, because netlify doesn't use GitHub Actions - it just runs npm run build

Since you are a collaborator on the project you should be able to push a branch to finos:static-build and it should work properly. Can you try that?

@mimiflynn mimiflynn mentioned this pull request Dec 18, 2024
@mimiflynn
Copy link
Member Author

@robmoffat Both PRs appear to be building successfully.

@robmoffat
Copy link
Member

@robmoffat Both PRs appear to be building successfully.

What I meant was, the builds aren't used for netlify.

@robmoffat
Copy link
Member

@robmoffat Both PRs appear to be building successfully.

If we're going to start using netlify previews, I think we also should use netlify for the live site as well, otherwise we're going to get caught out when a preview is different from the deployed version

@psmulovics
Copy link
Collaborator

Wait, I thought all finos sites are using netlify.

@robmoffat
Copy link
Member

Wait, I thought all finos sites are using netlify.

not this one it seems: https://github.com/finos/calendar?tab=readme-ov-file#live-environment

@psmulovics
Copy link
Collaborator

What would it take for this to run on netlify as well...? And, in the meantime, can we merge the static build PR?

@robmoffat
Copy link
Member

robmoffat commented Dec 19, 2024

What would it take for this to run on netlify as well...? And, in the meantime, can we merge the static build PR?

It's not super-hard, we're going to have to do a tiny bit of work with domain names. I'd like to sync with @TheJuanAndOnly99 to get this done

@psmulovics See also #221 which is based on this

@mimiflynn
Copy link
Member Author

Scope creep!

@mimiflynn mimiflynn merged commit 50d513a into finos:main Dec 19, 2024
11 checks passed
@mimiflynn mimiflynn deleted the static-build branch December 19, 2024 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants