Uncaught RangeError: Maximum call stack size exceeded

[dmitriykharchenko]

Hey,
HelmetWrapper uses deepEqual to compare props and of course, there are react children components with endless amount of nested objects.
This seems like a dangerous way to check props equality. Could we turn HelmetWrapper into PureComponent?

[srMarquinho]

Same here. Is there a solution to this?

[willmcpo]

Yeh same.

The work around for us was to not use react children to pass in <meta> <title> tags, and use the props way:

<Helmet
  title={pageTitle}
  meta={[
  {
    name: 'description',
    content: pageDescription,
   },
   ]}
/>

[lhtdesignde]

Just ran into this issue as well. Only saw it happening for Safari and Chrome browsers.

[nathanforce]

What is the reasoning behind the deep equality check? Per the React docs...

We do not recommend doing deep equality checks or using JSON.stringify() in shouldComponentUpdate(). It is very inefficient and will harm performance.

[jaredLunde]

I believe you can also work around this by applying a unique key prop to the <Helmet> component, for example <Helmet key={window.location.href}>. Perhaps a bit cleaner than using the props method above.

[dmitriykharchenko]

@jaredLunde not sure how that could speed up deepEqual in shouldComponentUpdate

react-helmet/src/Helmet.js

Line 93 in c102586

return !deepEqual(this.props, nextProps);

[jaredLunde]

@dmitriykharchenko Sorry, that was a reference to the Uncaught RangeError: Maximum call stack size exceeded error and presumably #373 (comment) - not anything perf related.

[dmitriykharchenko]

@jaredLunde this error happens because ReactHelmet compares props with deepEqual and when there are children in props it iterate over children as well and since those children are React components (not a plain objects) it simply overflows the stack. The workaround is to avoid providing any children to <Helmet/> component, but anyway, deepEqual is still bad.

[dmitriykharchenko]

So, I don't see any reason why <Helmet/> needs a key prop

[jaredLunde]

@dmitriykharchenko Gotcha! Well in that case, key solves the problem because deepEqual never gets called in shouldComponentUpdate as the component re-mounts instead of updating. Hence, no stack overflow. Was very useful in my case.

[dmitriykharchenko]

Hm, interesting. Do you have a link to read more about re-mounts of keyed elements?

[jaredLunde]

https://reactjs.org/docs/reconciliation.html#keys

Essentially, when the key changes it tells the reconciliation algorithm that the element in the new tree is different from the element in the old tree. If the new key wasn't in the children in old tree, a new component is mounted. If the new key was in the children in the old tree the element will be re-used.

[dmitriykharchenko]

Aha, you mean that you have to update that key on every render.
Yep, that seems faster than deepEqual, but still not quite clean as fix deepEqual or avoid children.

[jaredLunde]

Well, you only have to update the key when the underlying children change. In my case that happens when that particular view URL changes, but you can use any string representation. Maybe someone has a react-router match.params change which updates the title or meta information - that'd be a good spot to use key IMO. Anything where state affects the children.

[jaredLunde]

It might not work in some cases. Either way I think we're all in agreement that deepEqual is a bad practice in sCU.

[piotrblasiak]

This is a really bad bug - as the props way is not even the documented recommended way to pass metadata to helmet, and there is no other workaround.

[theKashey]

Just replace it by https://github.com/FormidableLabs/react-fast-compare

[ivan-kleshnin]

Why this is still not adressed?

[ivan-kleshnin]

1. key trick doesn't help in all cases
2. Library still crashes in 5.2.0

This bug is devastating. Had to switch to react-helmet-async which doesn't have this problem 😢

[tmbtech]

#402 was merged into master. I would like to get the roll-up PR merged and we can release a beta version for feedback.

[rockmandash]

I have the same problem...............
Why is this closed?

[kambing86]

I have the same problem...............
Why is this closed?

I believe it's merged to master branch, we need to wait them to release a new version

[rbalicki2]

FWIW I had this issue when running nightwatch / magellan tests. The offending code was in an HOC. The fix was to use an existing instance of helmet as follows:

+  const helmet = <Helmet><style key={styles.__hash}>{styles.__css}</style></Helmet>;
+
   const EnhancedComponent = props => (<Fragment>
-    <Helmet>
-      <style key={styles.__hash}>{styles.__css}</style>
-    </Helmet>
+    { helmet }

(I would suspect that let stylesDiv = <style>...</style>; ... <Helmet>{stylesDiv}</Helmet> might also fix the issue.)

Fingers crossed this will improve performance as well.

[catamphetamine]

@rockmandash Agree: this should be reopened due to not being fixed so far because otherwise it confuses people searching for this type of issue.

[tmbtech]

Merged in master, but not yet release. Reopening and will close once it gets published to NPM.

[dmitriykharchenko]

Well, I fixed that by removing helmet and implementing my own little lib.
It works pretty well with SSR including streaming rendering. If anybody interested, let me know, I'll open source it.

[donaldpipowitch]

Looking forward to a release 😍 We have this problem as well and just tracked down the issue.

[catamphetamine]

@donaldpipowitch
Think about this: the fix has been merged for a month now and the maintainters of the library still don't care at all.
They could have released this easy fix for a month now and you'd never have to waste any development work hours to track down this weird bug.
Still, they don't care and they continue to knowingly waste the time of developers all over the world just because the maintainers of this library are too lazy and ignorant.

[tmbtech]

Still, they don't care and they continue to knowingly waste the time of developers all over the world just because the maintainers of this library are too lazy and ignorant.

We are volunteers and I was on paternity leave (returned last tuesday). Please be civil, we are all human.

You are also welcome to pull from master, copy and paste the code into a vendor folder and use as is. Or use a different package entirely, react-helmet-async looks promising https://github.com/staylor/react-helmet-async.

[tmbtech]

Updating for visibility.

We had some organizational changes and I need to get permission in order to publish to NPM. While that is in progress, we have published to our internal nexus servers for testing.

Stay tuned. v.6.0.0-beta is coming.
We had to bump a major version because we removed the default export and migrated to rollup.js for better bundling support.

thank you all.

[kambing86]

@donaldpipowitch
Think about this: the fix has been merged for a month now and the maintainters of the library still don't care at all.
They could have released this easy fix for a month now and you'd never have to waste any development work hours to track down this weird bug.
Still, they don't care and they continue to knowingly waste the time of developers all over the world just because the maintainers of this library are too lazy and ignorant.

I can understand why this guy is frustrated, given that there is no new release for this package more than a year, but I don't agree on how he diss the maintainers or contributers.

I think you guys should have released a patched version for v5 without the rollup.

[theKashey]

I few months ago (when I failed to use this library) - I wrote another one. It's not a drop in replace due to different design (does not create title/meta tags), but could save your day - https://github.com/theKashey/react-push-channel

[raajnadar]

@kambing86 is right a patch version will be great at least we can publish the changes to our production application without any nasty hacks to fix this issue.

[gersomvg]

My application crashes frequently due to this bug. If this bug isn't resolved before our product launch, I'm forced to replace it with another lib / my own implementation 😕

I guess it's not too hard to release a v5 patch? Has nothing to do with the current work on v6 right?

[mandazi]

I came across this issue as well. Looking forward to the next release with the fix.

[szimek]

For some strange reason it happens just for one person in our office, so we haven't noticed this bug during development and tests. Any chance of releasing the beta version soon? :)

[raajnadar]

I got a temporary workaround for this issue.

1. Clone the master branch
2. Copy all the files that are inside the /react-helmet/src/
3. Paste that inside /your-project/src/temp/
4. Convert this code with import { Helmet } from 'react-helmet' into 'import { Helmet } from './temp/Helemt'

Make sure the import path is correct.

Once the fix is published to NPM you can delete the /src/temp folder and change the import

[nickensoul]

Hello guys.
Any news to just publish patched version to NPM?

[catamphetamine]

Remember how it all started in 2015?
So much promises, politeness and willingness to serve the community.
Naive.
erikras/react-redux-universal-hot-example#89
@tmbtech

How do you feel about adding React-Helmet to update titles and meta data? This component would allow developers to update the title/meta tags based on the view.
I'm part of the team that open sourced it. I can PR some code, but I wanted to check with you before I started writing code.
lmk, thanks.

It looks like they are pretty similar, the biggest advantage would be hands on support, I can always bubble up any issues to the team. :)
We are using React-Helmet in all of our new react websites, we are also seeking feedback so we can improve on our open source workflow. We plan to release more and the feedback would help tremendously.

Three years passed and no one cares anymore.
Reminds me of some clumsy accidental teenage male/female relationship.

Frontend was blooming at the time.
Now it's all starting to rot.

[donaldpipowitch]

Please everyone, give @tmbtech a rest or pay him money for working on this issue. Everyone can fork this lib and patch the code or switch to react-helmet-async or use an object for configuration. There are a lot of alternatives.

[saiichihashimoto]

Wow, people are rude.

[nickensoul]

Ok, I can to pay something. What the cost you’re talking about?
We just need a working package, and we know what the “open source” means.

[theKashey]

Dont forget - you may fork this repo and publish as @yourname/react-helmet.
The problem is already solved, fix is in the master branch - just not published to npm.

[tmbtech]

Hi all, Quick update.

@cwelch5 and I got the required permission to NPM. We will be publishing to NPM today or tomorrow.

because the thread is getting a little out of hand, I'm closing and locking the thread. I will be adding a code of conduct and other community support documentation. #424

Thank you all, and thank you for being apart of the community.

[cwelch5]

Thanks everyone, publishing is working now again. We should be able to post new NPM builds with ease.