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

SSR Experimental #8356

Closed
wants to merge 21 commits into from
Closed

Conversation

shashkovdanil
Copy link
Contributor

@shashkovdanil shashkovdanil commented Dec 2, 2023

Thank you for your contribution to the KodaDot - One Stop Shop for Polkadot NFTs.

👇 __ Let's make a quick check before the contribution.

PR Type

  • Bugfix
  • Feature
  • Refactoring

Context

  • Closes
  • Requires deployment <snek/rubick/worker>

Before submitting pull request, please make sure:

  • My contribution builds clean without any errors or warnings
  • I've merged recent default branch -- main and I've no conflicts
  • I've tried to respect high code quality standards
  • I've didn't break any original functionality

Optional

  • I've tested it at </ksm/collection>
  • I've tested PR on mobile
  • I've written unit tests 🧪
  • I've found edge cases

Did your issue had any of the "$" label on it?

  • Fill up your DOT address: Payout

Community participation

Screenshot 📸

  • My fix has changed something on UI; a screenshot is best to understand changes for others.

Copilot Summary

🤖[deprecated] Generated by Copilot at a264c75

The pull request improves the compatibility, performance, and readability of various components by using custom hooks and services to access browser APIs and environment variables. It also refactors some code to simplify logic and avoid reactivity issues. The main files affected are components/blog/BlogPost.vue, components/carousel/utils/useCarouselEvents.ts, components/chart/PriceChart.vue, components/collection/HeroButtons.vue, components/collection/unlockable/CountdownTimer.vue, components/collection/unlockable/UnlockableLoader.vue, components/common/ConnectWallet/ConnectWalletModal.vue, components/CookieBanner.vue, components/gallery/GalleryItemTabsPanel/GalleryItemTabsPanel.vue, components/MessageNotify.vue, components/Navbar.vue, components/profile/ProfileDetail.vue, components/base/MediaItem.vue, components/collection/unlockable/UnlockableHeroButtons.vue, components/CustomSubstackEmbed.vue, components/gallery/GalleryItemButton/GalleryItemShareBtn.vue, components/landing/MobileHeroBanner.vue, components/navbar/NotificationBoxButton.vue, components/navbar/ShoppingCartButton.vue, and components/search/SearchBar.vue.

🤖[deprecated] Generated by Copilot at a264c75

We're the masters of the browser, we don't need the window
We use custom hooks and services, to make our code more nimble
We check the server and the client, we avoid the errors of the document
We're the masters of the browser, we rock the vue component

@shashkovdanil shashkovdanil requested review from a team as code owners December 2, 2023 03:16
@shashkovdanil shashkovdanil requested review from yangwao and preschian and removed request for a team December 2, 2023 03:16
Copy link

netlify bot commented Dec 2, 2023

Deploy Preview for koda-canary ready!

Name Link
🔨 Latest commit 7d9b2cf
🔍 Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/656be2f108517300088bd2fc
😎 Deploy Preview https://deploy-preview-8356--koda-canary.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@reviewpad reviewpad bot added large Pull request is large waiting-for-review labels Dec 2, 2023
Copy link
Contributor

reviewpad bot commented Dec 2, 2023

Reviewpad Report

⚠️ Warnings

  • Please link an issue to the pull request

Copy link
Contributor

reviewpad bot commented Dec 2, 2023

AI-Generated Summary: This pull request contains updates to multiple files throughout the codebase with a consistent theme of refining the code to improve efficiency, robustness, compatibility with server-side rendering, and readability.

The dependencies in the pnpm-lock.yaml file have been modified with additions, deletions, and updates. In the Vue components, there is a significant shift in the way URLs, Window dimensions, and LocalStorage are accessed and used to meet the need for more reliable, safe, and efficient code execution.

Most importantly, the adaptation for server-side rendering (SSR) is visible in many files, through the use of conditional checks (like process.client) before accessing properties that are specific to the client-side, such as window, localStorage, document event listeners, and others. This helps to prevent errors during SSR where these objects are undefined.

In several places, the window.location and properties therein (like href, origin, pathname) have now been replaced to use the useRequestURL() hook, which appears to provide a more reliable and testable approach. Also, the window.innerWidth is replaced by the useWindowSize() hook for a more reactive handling of viewport changes, making the components responsive.

LocalStorage operations have also been made more null-safe by using optional chaining (?.) and importing localStorage from a separate module for more reliable handling.

Moreover, the import of heavy libraries and plugins has been moved to Vue onMounted hooks, leading to a lazy load pattern and potentially improving initial page loads, which can be highly beneficial for end-user experience.

Some noticeable changes have been made to alter method of async import for the web3Enable from '@polkadot/extension-dapp' module now only on the client-side, ensuring that runtime errors in contexts such as testing and SSR are mitigated.

The server-side rendering (ssr) previously set to false has also been made true in the nuxt.config.ts file and added a new module, '@pinia-plugin-persistedstate/nuxt'. Furthermore, a notable deletion of a 'piniaPersistedState.ts' file under the plugins directory, which used 'pinia' and 'pinia-plugin-persistedstate', should be looked out with caution as it might impact elsewhere in the code.

The newly added file useEnvironment.ts under composables directory, determines the runtime environment of the application. Another noteworthy addition of a browserAPIs.ts file in services directory exports constants that refer to different browser APIs like localStorage, sessionStorage, location, navigator, document, and history. Global usages of localStorage & window are now seen being replaced by the ones from this browserAPIs service.

Code has been fine-tuned for improved readability with changes to import statements, formatting, and breaking down complex logical checks into more straightforward syntax. Console logs have been added in some places presumably for debugging purposes.

The overall changes to components, services, and utilities are mostly about improving maintainability, robustness, SSR compatibility while some new dependencies added to the lock file hint at extending the functionality.

@shashkovdanil
Copy link
Contributor Author

shashkovdanil commented Dec 2, 2023

@roiLeo Hi, sorry I didn't ask you what the status of this task is, I just saw that it's been in Draft status for a few weeks and I wanted to do something interesting Friday night. The project is at least locally up and running, I'll need help with testing.

About this PR

I've been able to solve almost all of the problems you've encountered. I didn't use ClientOnly at all, pinia stores state is restored from localStorage but it can be placed in a cookie, also no dark mode bug.

The only problem I've found is that on netlify every second/third refresh of the page the same error like yours crashes
Screenshot 2023-12-02 at 03 30 38

UPDATE:

I solved problem on netlify. The only big problem left is that the tests are failing

@shashkovdanil
Copy link
Contributor Author

shashkovdanil commented Dec 2, 2023

cc @yangwao you'll be glad

Screenshot 2023-12-02 at 03 46 18

@preschian
Copy link
Member

cc @yangwao you'll be glad

nice, but I can't open the preview page

Screenshot 2023-12-02 at 6 52 15 PM

need to test it on the Cloudflare pages also

@shashkovdanil
Copy link
Contributor Author

cc @yangwao you'll be glad

nice, but I can't open the preview page

Screenshot 2023-12-02 at 6 52 15 PM

need to test it on the Cloudflare pages also

How can I do it? On netlify preview everything is fine

@shashkovdanil
Copy link
Contributor Author

hmm yesterday preview was fine...

@shashkovdanil
Copy link
Contributor Author

@preschian check it please https://deploy-preview-8356--koda-canary.netlify.app/

I think the error was caused by my last commit yesterday, now I did a revert and everything is ok

@shashkovdanil
Copy link
Contributor Author

With latest fixes:
Screenshot 2023-12-02 at 12 58 57

cc @yangwao

Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

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

  • Some of main components need ClientOnly (like darkmode)
  • Dropdowns looks weird
  • profile/wallet doesn't work on reload

same thing as #7725

@shashkovdanil
Copy link
Contributor Author

@roiLeo thanks! I'm checking

@shashkovdanil
Copy link
Contributor Author

@roiLeo could you give me more details about this please

  • Some of main components need ClientOnly (like darkmode)
  • Dropdowns looks weird

I fixed stores which connected with localStorage, it should be persist

@roiLeo
Copy link
Contributor

roiLeo commented Dec 2, 2023

* Some of main components need ClientOnly (like darkmode)

Screenshot 2023-12-02 at 14-56-34 KodaDot - NFT Market Explorer

* Dropdowns looks weird

Screenshot 2023-12-02 at 14-55-54 KodaDot - NFT Market Explorer

I fixed stores which connected with localStorage, it should be persist

@@ -0,0 +1,33 @@
export default function (customBreakpoint?: number) {
Copy link

Choose a reason for hiding this comment

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

Function default has 26 lines of code (exceeds 25 allowed). Consider refactoring.

@shashkovdanil
Copy link
Contributor Author

SSR affects performance in such a way that a user with poor internet will see content faster, i.e. instead of a white screen they will get html + css right away. It is also useful for SEO.

Btw Idk I got other results in Lighthouse:
this branch https://deploy-preview-8356--koda-canary.netlify.app/
desktop
Screenshot 2023-12-02 at 19 26 39

mobile
Screenshot 2023-12-02 at 19 31 58

canary https://canary.kodadot.xyz/
desktop
Screenshot 2023-12-02 at 19 28 07

mobile
Screenshot 2023-12-02 at 19 32 12

Checked in Google Chrome Incognito with disabled cache. If you don't even look at scores, you can see serious improvements in the metrics. Yes for mobile the overall score has decreased a bit (this is mainly due to the ssr images not affecting it), but it is worth paying attention to the metrics, they have improved significantly.

And about the size of the PR, yes it is really big, but there is no other way to do it. I just saw that @roiLeo also worked on SSR, so I thought it was important for the project


const isRmrk = computed(() => prefix.value === 'rmrk')
onMounted(() => {
Copy link

Choose a reason for hiding this comment

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

Identical blocks of code found in 2 locations. Consider refactoring.

const isAssetHub = computed(
() => prefix.value === 'ahk' || prefix.value === 'ahp', //|| prefix.value === 'ahr'
)
watch(prefix, () => {
Copy link

Choose a reason for hiding this comment

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

Identical blocks of code found in 2 locations. Consider refactoring.

@shashkovdanil
Copy link
Contributor Author

shashkovdanil commented Dec 3, 2023

Strange problem with tests, I repeated the same environment like on CI. Now only one test failed transfer.spec.ts
Screenshot 2023-12-03 at 02 06 12

Copy link

codeclimate bot commented Dec 3, 2023

Code Climate has analyzed commit 7d9b2cf and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1
Duplication 2

View more on Code Climate.

Copy link

sonarcloud bot commented Dec 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.9% 0.9% Duplication

@shashkovdanil
Copy link
Contributor Author

Tests are green

@shashkovdanil
Copy link
Contributor Author

shashkovdanil commented Dec 3, 2023

Hey @roiLeo @preschian I know the PR turned out gigantic and realize it's risky to merge it into the main branch. So I won't mind, and I won't be upset if you close it. I did it on my own initiative and the task was really interesting. Below I will write a list of tricky problems that I managed to solve, as it may be useful in the future:

  1. Unable to initialise options more than once error on netlify preview. Solution is here
  2. Tests often crash because the test is trying to click a button, the button is there on the page, but the click doesn't work. The problem is because the button is already on the page, but the hydration process has not finished yet. Solution: use await page.waitForLoadState('networkidle') after await page goto()
  3. Problem with computed and watch. It doesn't work properly after ssr. It's not our problem it's bug in Vue Hydration not updating DOM of img-src and v-html with new client-side values vuejs/core#9033. Solution: use watchPostEffect or onMounted to init correct value and watch to watch for value.
  4. All pinia stores that are associated with localStorage must be with persist: true and don't forget to install @pinia-plugin-persistedstate/nuxt
  5. If you need to get the data of the current url, you need to useRequestURL instead of window.location
  6. Changing icons depending on the theme or screen resolution is better done with CSS. For example, if you want to change the color of the icon depending on the theme, you can use filter: invert(0 | 1)
  7. Hack to fix problem with tests with tslib. Issue is here Could not load node_modules/tslib/tslib.es6.js/tslib.es6.js when using @nuxt/apollo & @vueuse/motion/nuxt together nuxt/nuxt#19265. Solution is cd .output/server/node_modules/tslib; npm pkg set 'exports[.].import.node'='./tslib.es6.mjs' just run this command after build command
  8. Use useState for state management in Nuxt https://nuxt.com/docs/getting-started/state-management

If the activation of ssr is still of interest to you, please let me know and I'll look into it. I have some ideas on how to break this PR into many smaller ones. For example,

PR1. Replace the use of browser APIs/client APIs (like I did in the browserAPIs file)
PR1.1. Replace the use of localStorage/sessionStorage
PR1.2. Replace the use of window
PR1.3. Replace the use of location
PR1.4. Replace the use of navigator and document
PR1.5. Close behind process.client plugins
PR1.6. Import during onMounted for client libraries like Chart.js

PR2. Add await page.waitForLoadState('networkidle') to tests and command cd .output/server/node_modules/tslib; npm pkg set 'exports[.].imports.node'='./tslib.es6.mjs'

PR3. Integration pinia with localStorage (module @pinia-plugin-persistedstate/nuxt and persist: true)

PR4. Find places where html attributes (like src for img tag) are changed using computed/watch and use there useStore and watchPostEffect. Also replace changes using JS with CSS where possible

PR5. Here we can change ssr flag to true in nuxt.config

@preschian
Copy link
Member

@preschian check it please deploy-preview-8356--koda-canary.netlify.app

I think the error was caused by my last commit yesterday, now I did a revert and everything is ok

It works now 👍 could you test with Cloudflare pages? I already added this branch to our cf-pages, but it didn't work. Try to integrate your forked repo with Cloudflare pages, it's free https://pages.cloudflare.com/. Just to make sure the build on cf-pages was ok

@preschian
Copy link
Member

I know the PR turned out gigantic and realize it's risky to merge it into the main branch.
I have some ideas on how to break this PR into many smaller ones.

I always like the ideas about gradual release 👍

PR5. Here we can change ssr flag to true in nuxt.config

This is still too big because it affects all endpoints. To minimize the risk, I think we can enable it by endpoints:

PR -> ssr only on /terms-of-use and /privacy-policy
PR -> + ssr on /blog/*
PR -> + ssr on /ahp/drops
so on...

@shashkovdanil
Copy link
Contributor Author

shashkovdanil commented Dec 4, 2023

@preschian btw ye, I can wrap client side pages in <ClientOnly>

Update

I've found this, so we can manage ssr for pages in config

image

@shashkovdanil
Copy link
Contributor Author

shashkovdanil commented Dec 4, 2023

Then I think this PR can be closed and I'll create an epic task with small subtasks. Does that sound good to you guys?

@roiLeo @preschian

@roiLeo roiLeo self-requested a review December 5, 2023 08:37
Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

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

wow, I can see that this branch start to look good there is a lot of progress compared to #7725

Now we have to test that nothing breaks: on first load, with cache, without cache, with reload etc..

Note: I still see light mode logo on dark mode 2 sec before it change

@@ -6,7 +6,10 @@
width="740"
height="1000"
alt="" />
<img :src="landingImage[0]" class="landing-shapes" alt="" />
<img
src="/landing-shape-header-left-light.svg"
Copy link
Contributor

Choose a reason for hiding this comment

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

oh this is new

Comment on lines +48 to +50
@include ktheme() {
filter: theme('landing-shape-header');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why put code in this component?
https://caniuse.com/css-filters global 97% ok

email: '#000000',
text: '#000000',
},
} as const
Copy link
Contributor

Choose a reason for hiding this comment

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

as const?

@@ -7,7 +6,13 @@ import { useChainStore } from '@/stores/chain'

// }

export default defineNuxtPlugin(() => {
export default defineNuxtPlugin(async () => {
if (!process.client) {
Copy link
Contributor

Choose a reason for hiding this comment

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

rename file to polkadot.client.ts for only client side

Comment on lines +8 to +10
if (!process.client) {
return
}
Copy link
Contributor

Choose a reason for hiding this comment

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

pwa.client.ts

@shashkovdanil
Copy link
Contributor Author

@roiLeo to avoid flash logo for different themes we have to use css and don't use js for styling anymore.

But anyway this PR is so big, I'll separate it to many parts, I think we will have ssr within 1-2 weeks

@shashkovdanil shashkovdanil mentioned this pull request Dec 8, 2023
8 tasks
@shashkovdanil
Copy link
Contributor Author

#8489

@preschian preschian mentioned this pull request Jan 15, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
large Pull request is large waiting-for-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants