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

Export QRCodeSVG and QRCodeCanvas directly. #56

Closed
wants to merge 1 commit into from

Conversation

realityking
Copy link
Contributor

I was thinking a bit more about #49 and how to publish this component as an ESM. The best way to guarantee compatibility between CommonJs and ESM right now is to only use named exports.

This PR is obviously a breaking but it not only adds the ability to eventually use ESM it also removes a level of indirection and helps editors discover the propTypes for the components. Once shipping with ESM it should also allow bundles to tree shake out the unused module.

WDYT?

@zpao
Copy link
Owner

zpao commented Sep 2, 2018

Thanks for bringing this up. I was thinking about this too, going back to when I added the SVG version.

helps editors discover the propTypes for the components

To be fair, I think we could just add propTypes to the wrapper component and get the same thing :)

Now to the real point… it's kind of an awkward API. I was tempted to split out of the single file and just expose qrcode.react/canvas and qrcode.react/svg instead of 2 exports on a single module, though I guess it's not terribly different. No idea how that would work with building CJS & ES modules though, presumably some of the same problems?

That said, seems like a reasonable path forward. I guess let's do it. Does this mean we'd be able to keep the module field in #49?

@realityking
Copy link
Contributor Author

To be fair, I think we could just add propTypes to the wrapper component and get the same thing :)

True 😄

That said, seems like a reasonable path forward. I guess let's do it. Does this mean we'd be able to keep the module field in #49?

The difference is named vs default exports. Named exports work fine with Webpack when mixing CJS and ESM, default exports don't.

@realityking
Copy link
Contributor Author

@zpao Do you wanna continue with this? I'd be happy to pick up #49 if this gets merged.

@realityking
Copy link
Contributor Author

@zpao Any news on this one?

@realityking
Copy link
Contributor Author

Rebased for current master.

@realityking
Copy link
Contributor Author

Hi @zpao,

I dug this old PR out as I'd still love to see it merged.

  1. We're much closer to mainstream builds being able to use ESM. This is a good step towards that.
  2. It reduces code size at least a little bit which is always nice.

Of course the true value will be once there's an ESM build as it'd allow tree-shaking the unused renderer.

This makes it possible to ship an ESM, potentially allowing Webpack to tree shake out the unused component.

It also removes a level of indirection and helps editors discover the propTypes for the components.
@meabed
Copy link

meabed commented Dec 15, 2021

@realityking I have this fixes here too https://github.com/devmehq/react-qr-code

@zpao
Copy link
Owner

zpao commented Mar 6, 2022

Will be doing this in #174. Leaving the default export in for now. Sorry I never got to it sooner!

@zpao zpao closed this Mar 6, 2022
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