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

feat: enable ssr on /privacy-policy #9155

Draft
wants to merge 42 commits into
base: main
Choose a base branch
from
Draft

Conversation

preschian
Copy link
Member

@preschian preschian commented Jan 27, 2024

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

Improved page speed scores on /privacy-policy. But not so much, it still has areas of improvement. (some components such as Navbar still client side)

Next steps:

  • I will fix ci for libs/ui in a separate PR. Turns out not related to this PR, issue here libs/ui @google/model-viewer build error #9174
  • For temporary, I create a new default-ssr layout. To test the SSR page. At the moment, some components still do not work on SSR. such as components under <ClientOnly> on the default-ssr layout
  • I disabled the source map for now and will enable it again later.

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

refactor: rename plugin files to indicate client-side usage
feat: add new posts to posts.json
fix: adjust localStorage usage in identity store
fix: adjust window object usage in chain and ipfs config
fix: adjust extension usage for client-side process
…and '@polkadot/x-global' to transpile list and update '@polkadot/x-global' version to 12.6.2
… vue components

fix(package.json): downgrade eslint-plugin-unicorn, eslint-plugin-vue, glob, husky, jsdom, lint-staged versions
chore(package.json): remove unused @types/jest and @polkadot/x-global dependencies
refactor(default.vue): remove ClientOnly wrapper from Navbar, Footer, CookieBanner, and Buy components
feat(privacy-policy.vue): set page layout to default-ssr
fix(nuxt.config.ts): disable sourcemap in vite build configuration
feat: enable ssr on `/privacy-policy`
Copy link

netlify bot commented Jan 27, 2024

Deploy Preview for koda-canary ready!

Name Link
🔨 Latest commit 270c5b5
🔍 Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/65fa544804b4960008503baa
😎 Deploy Preview https://deploy-preview-9155--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.

Copy link
Contributor

reviewpad bot commented Jan 27, 2024

AI-Generated Summary: This pull request includes significant changes in multiple files influencing client-side limiting checks, server-side rendering, updates to JSON data, development and build configurations, and a few changes in typescript configurations.

  1. Many files now exert a constraint on operations dependent on browser APIs, like localStorage and window.location.hostname, restricting them to be handled only in a client-side scenario. This increases the robustness of the codebase when executed in diverse environments.

  2. An SSR principle has been implemented into various components. The default-ssr.vue file has been added providing a base layout template for SSR. Some other files have also been renamed to signify they are intended for client-side operation eg.keyboardEvents.ts to keyboardEvents.client.ts.

  3. On the data aspect, posts.json has been updated with three new posts while removing four older entries.

  4. prefix.global.ts and utils/chain.ts were updated by adding a process.client condition to the conditional checks.

  5. package.json has witnessed version updates for several dependencies and some packages were removed from devDependencies.

  6. For the project setup, nuxt.config.ts was altered concerning sourcemaps, dependency optimization, module transpilation, and a possible server-side rendering enablement.

  7. Furthermore, a few z-index changes in CSS and certain model-viewer library import changes were made.

  8. A new server-side TypeScript configuration was made in the server directory.

In sum, this update has a widespread impact on the application, concerning client and server-side operation, development environment configurations, data updates, and library dependency versions.

@reviewpad reviewpad bot added the large Pull request is large label Jan 27, 2024
@yangwao
Copy link
Member

yangwao commented Feb 1, 2024

I would put it to other release if so so nothing goes wrong and well tested as some things appeared

@preschian
Copy link
Member Author

The build failed again after sync from the main branch. It seems unstable. Let's keep it for several weeks to test it. At the moment, I will change it into a draft PR

@preschian preschian marked this pull request as draft February 2, 2024 03:20
@preschian
Copy link
Member Author

preschian commented Feb 2, 2024

After I removed the resolutions from package.json it works again. so, blocked by this https://github.com/vuejs/core/issues/10098

@prury prury removed the S-works-for-me-✅ qa-guild has tested PR from end user perspective and functionality worked label Feb 2, 2024
Copy link

sonarcloud bot commented Feb 5, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@preschian
Copy link
Member Author

After I removed the resolutions from package.json it works again. so, blocked by this https://github.com/vuejs/core/issues/10098

fiuuh, finally, merged https://github.com/vuejs/core/pull/10184

@yangwao
Copy link
Member

yangwao commented Mar 12, 2024

So it's worth pursuing SSR as apparently doesn't add that much score on pagespeed?

image
vuejs/core#10184

Does it improve any of following?

@preschian preschian added the S-stale pull request which haven't seen any changes for last 3 days, with this labels will be close in 7days label Mar 12, 2024
@preschian
Copy link
Member Author

So it's worth pursuing SSR as apparently doesn't add that much score on pagespeed?
Does it improve any of following?

IMO yes. We can forget about this unless we care about the score.

It can help improve the score, especially LCP. Fyi: https://web.dev/articles/optimize-lcp#server-side-rendering.

Based on the report from our Cloudflare pages, we have good scores on CLS and FID but not on LCP. SSR is the first step to optimising LCP. If we still need to reach good scores, we must check other metrics, such as javascript size on the client side. But at least we need to land SSR first.

image

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 6172 lines exceeds the maximum allowed for the inline comments feature.

Copy link

socket-security bot commented Mar 17, 2024

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

View full report↗︎

Copy link

codeclimate bot commented Mar 20, 2024

Code Climate has analyzed commit 270c5b5 and detected 0 issues on this pull request.

View more on Code Climate.

Copy link

sonarcloud bot commented Mar 20, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
large Pull request is large S-blocked-✋ S-stale pull request which haven't seen any changes for last 3 days, with this labels will be close in 7days waiting-for-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problems with Nitro x Cloudflare Pages
5 participants