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

Fix gatsby-plugin-react-helmet for v6.0.0 #10578

Merged
merged 2 commits into from
Dec 20, 2018
Merged

Fix gatsby-plugin-react-helmet for v6.0.0 #10578

merged 2 commits into from
Dec 20, 2018

Conversation

Ehesp
Copy link
Contributor

@Ehesp Ehesp commented Dec 20, 2018

Description

In version 6.0.0 of react-helmet, the default export has been removed. There is now only a named export, e.g.

import { Helmet } from 'react-helmet';

In v5.x.x, both a default export and named are exported. Change is here:

nfl/react-helmet@20ea385#diff-cc54072daf5278847980b841520c8fffL287

This is a backwards compatible change. v5 has an annoying bug (nfl/react-helmet#373) which v6 seems to fix.

Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for addressing this!

@DSchau
Copy link
Contributor

DSchau commented Dec 20, 2018

I can't actually seem to get this to successfully build with gatsby-default-starter, the changes in this PR, and then react-helmet@6 (currently just a beta?).

error Building static HTML for pages failed

See our docs page on debugging HTML builds for help https://gatsby.app/debug-html

   8 | 	else
   9 | 		root["lib"] = factory(root["@reach/router"], root["core-js/modules/es6.array.iterator"], root["core-js/modules/es6.array.sort"], root["core-js/modules/es6.function.name"], root["core-js/modules/es6.map"], root["core-js/modules/es6.object.assign"], root["core-js/modules/es6.regexp.constructor"], root["core-js/modules/es6.regexp.split"], root["core-js/modules/es6.regexp.to-string"], root["core-js/modules/es6.string.ends-with"], root["core-js/modules/es6.string.iterator"], root["core-js/modules/web.dom.iterable"], root["fs"], root["lodash"], root["path"], root["react"], root["react-dom/server"], root["react-helmet"]);
> 10 | })(this, function(__WEBPACK_EXTERNAL_MODULE__reach_router__, __WEBPACK_EXTERNAL_MODULE_core_js_modules_es6_array_iterator__, __WEBPACK_EXTERNAL_MODULE_core_js_modules_es6_array_sort__, __WEBPACK_EXTERNAL_MODULE_core_js_modules_es6_function_name__, __WEBPACK_EXTERNAL_MODULE_core_js_modules_es6_map__, __WEBPACK_EXTERNAL_MODULE_core_js_modules_es6_object_assign__, __WEBPACK_EXTERNAL_MODULE_core_js_modules_es6_regexp_constructor__, __WEBPACK_EXTERNAL_MODULE_core_js_modules_es6_regexp_split__, __WEBPACK_EXTERNAL_MODULE_core_js_modules_es6_regexp_to_string__, __WEBPACK_EXTERNAL_MODULE_core_js_modules_es6_string_ends_with__, __WEBPACK_EXTERNAL_MODULE_core_js_modules_es6_string_iterator__, __WEBPACK_EXTERNAL_MODULE_core_js_modules_web_dom_iterable__, __WEBPACK_EXTERNAL_MODULE_fs__, __WEBPACK_EXTERNAL_MODULE_lodash__, __WEBPACK_EXTERNAL_MODULE_path__, __WEBPACK_EXTERNAL_MODULE_react__, __WEBPACK_EXTERNAL_MODULE_react_dom_server__, __WEBPACK_EXTERNAL_MODULE_react_helmet__) {
     |  ^
  11 | return 


  WebpackError: Invariant Violation: Minified React error #130; visit https://reactjs.org/docs/error-decoder.html?inv

I'll investigate this a bit further.

@DSchau
Copy link
Contributor

DSchau commented Dec 20, 2018

Yeah - I can't seem to get this to build whatsoever.

Did you have any luck? Simply updating to [email protected] seems to break the build, even with this plugin's changes. In fact, it seems to fail before it even invokes the SSR APIs

Ha - would have to change the src/ directory to reflect this new pattern too 🙃

Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

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

OK! Able to validate this finally.

We'll have to make changes to the starters since they'll be incompatible with react-helmet@6 using the default export.

Before we get this merged, we should probably also tweak some documentation. I'll take a look at that and PR to this branch if that's alright!

@Ehesp Ehesp requested a review from a team as a code owner December 20, 2018 16:17
@Ehesp Ehesp requested a review from a team December 20, 2018 16:17
@Ehesp Ehesp requested a review from a team as a code owner December 20, 2018 16:17
@DSchau DSchau merged commit 99b57c1 into gatsbyjs:master Dec 20, 2018
@gatsbot
Copy link

gatsbot bot commented Dec 20, 2018

Holy buckets, @Ehesp — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. (Currently we’ve got a couple t-shirts available, plus some socks that are really razzing our berries right now.)
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

@Ehesp
Copy link
Contributor Author

Ehesp commented Dec 21, 2018

Hey sorry was in a rush yesterday. Yeah 6.0 is in beta but there was a very annoying issue it fixed. Thanks for sorting 👍

gpetrioli pushed a commit to gpetrioli/gatsby that referenced this pull request Jan 22, 2019
## Description

In version 6.0.0 of react-helmet, the default export has been removed. There is now only a named export, e.g.

```
import { Helmet } from 'react-helmet';
```

In v5.x.x, both a default export and named are exported. Change is here:

nfl/react-helmet@20ea385#diff-cc54072daf5278847980b841520c8fffL287

This is a backwards compatible change. v5 has an annoying bug (nfl/react-helmet#373) which v6 seems to fix.
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.

2 participants