Re-add Helmet as a default export

[okcoker]

I was in the process of upgrading some packages related to react-redux and react-redux-router (now connected-react-router) and noticed I was getting an "Uncaught RangeError: Maximum call stack size exceeded" as described here. After many hours of debugging I found it was coming from react-helmet.

It seemed like upgrading to 6.0.0 fixed the issue but I noticed 6.0.0 removed Helmet as a default export.

Bundle with Rollup instead of Webpack - As a result, the default export was removed and Helmet must now be imported as a named component

This would require me to change all imports within my app or use some magical alias webpack thing to keep the same import statements. I'm not sure why Rollup would necessitate a change to the API at all but I've added the default export back (in addition to the named export) for backwards compatibility for current v6 users and better forwards compatibility from v5 -> v6.

[CLAassistant]

All committers have signed the CLA.

[codecov]

Codecov Report

Merging #547 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #547   +/-   ##
=======================================
  Coverage   96.91%   96.91%           
=======================================
  Files           3        3           
  Lines         292      292           
=======================================
  Hits          283      283           
  Misses          9        9           

Impacted Files	Coverage Δ
src/Helmet.js	96.42% <ø> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10b1800...93e117f. Read the comment docs.

[cwelch5]

Thank you for supporting our project. According to #395 removing the default export was necessary as it didn't allow Rollup to work well.

I would hope a codemod could be a lighter solve for changing your imports. Sorry for the inconvenience.

[bdougherty]

@cwelch5 do you know specifically what the issue is? I've never had a problem with default exports and Rollup.

[cwelch5]

Thanks @bdougherty , I suppose if it could be tested, there may not be harm in having both. I'll give it a few sanity checks and perhaps this could work.

[cwelch5]

Thank you @okcoker for the contribution!

[realityking]

FWIW the reason I had removed the default export in my previous PR is related to how ESM modules get converted to CommonJS by Webpack.

Both Webpack and rollup will transform an ESM module that has both a named export and a default export to a CommonJS module that exports default as a property. In the case of react-helmet that looks like this:

exports.Helmet = HelmetExport;
exports.default = HelmetExport;

Now you have a situation where import Helmet from 'react-helmet' works but const Helmet = require('react-helmet') does not work. Luckily we've been using the named export in our project so this doesn't affect us. It is however a significant footgun.

See this issue for an example of a project where it did end up breaking our build: civiccc/react-waypoint#238

[okcoker]

Now you have a situation where import Helmet from 'react-helmet' works but const Helmet = require('react-helmet') does not work.

Yes this makes sense as you'd probably have to do const Helmet = require('react-helmet').default which is a bit annoying.

See this issue for an example of a project where it did end up breaking our build: civiccc/react-waypoint#238

You asked if there's any packages here doing es and cjs and my first thought is react-router where they have a main and a module key in the package.json. Maybe this package can do something similar.

Ultimately I feel import { Helmet } from 'react-helmet' is not correct because Helmet is in fact, the "default" of this package and should be imported as so. Importing a named export in this manner also gives the impression that there would be other named imports I can use which isn't the case here (but even if there were, I believe Helmet should still be the default, ie import Helmet, { anotherImport } from 'react-helmet'). I don't think Webpack/Rollup configuration issues shouldn't affect the way the language is being written. It seems like a band-aid solution (similar to this PR) although I'll admit that may be purely subjective.

I'm sure there are some resolution/alias options within both bundlers to make this a bit easier for the CJS environment but going forward, I would think import syntax should be priority for anything browser-related and especially since Node will have them by default in v14.

[ghost]

Importing a named export in this manner also gives the impression that there would be other named imports I can use which isn't the case here

This is the strongest argument I've seen mentioned as to keeping the default export. My suggestion when taking a new major is to always review the CHANGELOG first so you don't spend hours debugging in the first place. Unsolicited advice perhaps but worth keeping in mind so you can save yourself some time in the future.