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

Improve support for modern ESM build tools #210

Merged
merged 1 commit into from Jan 15, 2021
Merged

Improve support for modern ESM build tools #210

merged 1 commit into from Jan 15, 2021

Conversation

valin4tor
Copy link
Contributor

@valin4tor valin4tor commented Jan 14, 2021

What is the motivation for this pull request?

I had installed html-react-parser in a project using Snowpack, but found I could only access the default export, and not domToReact, htmlToDOM or attributesToProps.

What is the current behavior?

Snowpack - which uses esbuild internally - creates native ES module bundles which can be directly imported from the browser. Due to the way html-react-parser exports its modules, the following lines:

// support CommonJS and ES Modules
module.exports = HTMLReactParser;
module.exports.default = HTMLReactParser;

were being converted into:

// support CommonJS and ES Modules
var htmlReactParser = HTMLReactParser;
var _default = HTMLReactParser;
htmlReactParser.default = _default;

export default htmlReactParser;

...because esbuild was unable to detect the non-default exports.

What is the new behavior?

This PR adds explicit named exports, which allows them to be converted to native ESM with tooling such as esbuild.

CommonJS tooling is unaffected as the export structure remains the same.

Snowpack/esbuild now outputs the following after this patch:

// support CommonJS and ES Modules
var htmlReactParser = HTMLReactParser;
var _default = HTMLReactParser;

// better support for ESM build tools
var domToReact_1$1 = domToReact_1;
var htmlToDOM_1 = htmlToDom;
var attributesToProps_1$1 = attributesToProps_1;
htmlReactParser.default = _default;
htmlReactParser.domToReact = domToReact_1$1;
htmlReactParser.htmlToDOM = htmlToDOM_1;
htmlReactParser.attributesToProps = attributesToProps_1$1;

export default htmlReactParser;
export { attributesToProps_1$1 as attributesToProps, domToReact_1$1 as domToReact, htmlToDOM_1 as htmlToDOM };

Checklist:

  • Tests - not added as existing tests cover CJS usage.
    • Could add tests using esbuild to bundle but this may overcomplicate things - thoughts?

@coveralls
Copy link

coveralls commented Jan 14, 2021

Coverage Status

Coverage remained the same at 100.0% when pulling ca0340a on davidbailey00:patch-1 into bfba80e on remarkablemark:master.

@remarkablemark remarkablemark self-assigned this Jan 14, 2021
index.js Outdated
@@ -35,3 +35,8 @@ HTMLReactParser.attributesToProps = attributesToProps;
// support CommonJS and ES Modules
module.exports = HTMLReactParser;
module.exports.default = HTMLReactParser;

// better support for ESM build tools
exports.domToReact = domToReact;
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for opening this PR @davidbailey00! Unfortunately these changes will cause the UMD build to break.

You can verify this by running npm run build:

$ npm run build

> html-react-parser@1.1.2 build /Users/mark/Code/npm/html-react-parser
> rollup --config


index.js → dist/html-react-parser.js...
(!) Mixing named and default exports
https://rollupjs.org/guide/en/#outputexports
The following entry modules are using named and default exports together:
index.js

Consumers of your bundle will have to use chunk['default'] to access their default export, which may not be what you want. Use `output.exports: 'named'` to disable this warning
created dist/html-react-parser.js in 515ms

index.js → dist/html-react-parser.min.js...
(!) Mixing named and default exports
https://rollupjs.org/guide/en/#outputexports
The following entry modules are using named and default exports together:
index.js

Consumers of your bundle will have to use chunk['default'] to access their default export, which may not be what you want. Use `output.exports: 'named'` to disable this warning
created dist/html-react-parser.min.js in 1.1s

And then opening examples/index.html:

$ open examples/index.html

You should see the error in your browser Console:

Uncaught TypeError: window.HTMLReactParser is not a function
    at renderOutput (index.html:12)
    at index.html:19

Any reason you can't do this?

const parse = require('html-react-parser');

parse.domToReact;
parse.htmlToDOM;
parse.attributesToProps;

Copy link
Contributor Author

@valin4tor valin4tor Jan 15, 2021

Choose a reason for hiding this comment

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

Thanks for pointing out this issue as I didn't spot it myself! I've pushed an alternative approach in the form of adding a new entry point for ES modules, which I've verified also works as expected with Snowpack and doesn't change the CJS or UMD approaches. Curious to see what you think of this instead :)

Any reason you can't do this?

I can do this yes, or an ESM equivalent:

import HTMLReactParser from 'html-react-parser';
const { domToReact, htmlToDOM } = HTMLReactParser;

However this is a little annoying when I don't actually want to use the default export. In addition, it doesn't line up with the type definitions which expect domToReact etc to be imported as named imports, rather than being used as properties of the default import.

Copy link
Owner

@remarkablemark remarkablemark left a comment

Choose a reason for hiding this comment

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

The alternative looks good! Thanks for the quick turnaround @davidbailey00

@remarkablemark remarkablemark merged commit 4caa88c into remarkablemark:master Jan 15, 2021
@remarkablemark
Copy link
Owner

Published v1.2.0:

npm:

npm i html-react-parser@1.2.0

Yarn:

yarn add html-react-parser@1.2.0

@irvinebroque
Copy link

With unbundled development enabled in Snowpack, this ends up compiling down to:

import React from '/_snowpack/pkg/react.v16.14.0.js';
import reactProperty from '/_snowpack/pkg/react-property.v1.0.1.js';
import require$$0 from '/_snowpack/pkg/style-to-js.v1.1.0.js';
import htmlToDOM$1 from '/_snowpack/pkg/html-dom-parser.v1.0.1.js';

var styleToJS = require$$0.default;

Snowpack config:

    target: "chrome80",
    treeshake: true,
    preload: true,
  }

Seems like this and related modules would benefit from fully migrating to ES modules - right now this has an modules entrypoint, but the exported build contains non-modules. Easy to end up with scenarios where build tools get confused by this.

@remarkablemark
Copy link
Owner

@irvinebroque would you like to open a PR?

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.

None yet

4 participants