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

separate context for different pages with pathname #106

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mjBayati
Copy link

@mjBayati mjBayati commented Dec 9, 2022

fix issue #97.
by separating contexts of each page on rendering root element in gatsby-ssr

@me4502
Copy link
Owner

me4502 commented Dec 11, 2022

This change appears to make sense, I just have two concerns;

  • If anything relies on this data (unlikely)
  • Whether this introduces a "memory leak" for larger sites, as it's now creating an object for every individual path. For a site that stores a bunch of metadata in the head across 100k pages for example, that's potentially a substantial increase in memory

@mjBayati
Copy link
Author

thanks for your review, especially about your wise comment on memory leak
i have two question:
first, I don't understand your point in first item: If anything relies on this data (unlikely), could you please explain it more?

second, I think for using helemt async provider with 100k pages, programmer should separate it's application pages to different builds, because loading this amount of data, without using different helemt context, create memory leak in application itself. some related issue is opened yet.

by the way, what i understand from gatsby-build and gatsb-ssr api's, the best solution is to separate contexts, but if there is better solution to avoid memory leak, I'm open to apply your suggestions .

thanks,

@mjBayati
Copy link
Author

is there any update ?
please inform me if you have any consideration on my pull request

@me4502
Copy link
Owner

me4502 commented Dec 20, 2022

My main concern here is that this update would break any existing sites with a large number of pages, if it's causing major memory leaks. If it doesn't actually cause a large increase for a few hundred thousand pages with a normal amount of meta tags being added, I'd be okay with the change as-is

@@ -2,7 +2,7 @@ import { GatsbySSR, RenderBodyArgs, WrapRootElementNodeArgs } from 'gatsby';
import React from 'react';
import { HelmetProvider, HelmetServerState } from 'react-helmet-async';

const context: { helmet?: HelmetServerState } = {};
const context: {[pathname: string]: { helmet?: HelmetServerState }} = {};
Copy link

Choose a reason for hiding this comment

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

Is this correctly typed?

On Line #14 we're checking if (helmet) and then accessing helmet.base but it's actually helmet.helmet.base now?

npm ERR! src/gatsby-ssr.tsx(16,39): error TS2339: Property 'title' does not exist on type '{ helmet?: HelmetServerState | undefined; }'.
npm ERR! src/gatsby-ssr.tsx(18,20): error TS2339: Property 'priority' does not exist on type '{ helmet?: HelmetServerState | undefined; }'.
npm ERR! src/gatsby-ssr.tsx(19,20): error TS2339: Property 'meta' does not exist on type '{ helmet?: HelmetServerState | undefined; }'.
npm ERR! src/gatsby-ssr.tsx(20,20): error TS2339: Property 'link' does not exist on type '{ helmet?: HelmetServerState | undefined; }'.
npm ERR! src/gatsby-ssr.tsx(21,20): error TS2339: Property 'style' does not exist on type '{ helmet?: HelmetServerState | undefined; }'.
npm ERR! src/gatsby-ssr.tsx(22,20): error TS2339: Property 'script' does not exist on type '{ helmet?: HelmetServerState | undefined; }'.
npm ERR! src/gatsby-ssr.tsx(23,20): error TS2339: Property 'noscript' does not exist on type '{ helmet?: HelmetServerState | undefined; }'.
npm ERR! src/gatsby-ssr.tsx(32,34): error TS2339: Property 'htmlAttributes' does not exist on type '{ helmet?: HelmetServerState | undefined; }'.
npm ERR! src/gatsby-ssr.tsx(33,34): error TS2339: Property 'bodyAttributes' does not exist on type '{ helmet?: HelmetServerState | undefined; }'.

Copy link

Choose a reason for hiding this comment

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

Actually this typing is probably correct but we should access context on onRenderBody like this:

export const onRenderBody: GatsbySSR['onRenderBody'] = ({
  pathname,
  setHeadComponents,
  setHtmlAttributes,
  setBodyAttributes,
}: RenderBodyArgs): void => {
  const { helmet } = context[pathname]

@oskari
Copy link

oskari commented Mar 10, 2023

I implemented these changes and my own remarks on my own fork as I needed to get this fixed asap. We have about 30K pages but we're not really limited by memory afaik. So far everything is working but we haven't made any extensive testing

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.

3 participants