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

♻️ add back ts-compatible default export #374

Closed
wants to merge 1 commit into from
Closed

♻️ add back ts-compatible default export #374

wants to merge 1 commit into from

Conversation

one19
Copy link

@one19 one19 commented Jun 27, 2018

Hi there!

A few days ago, a change was introduced that broke TS default import expectations.

The import code specified in the readme isn't valid right now, I believe after this commit was added:
4c15c32

If doing import * as ReactQuill from 'react-quill'; the returned class won't have constructor or call methods, and it'll cease to work as advertised.

I'm not sure if you're cool with this solution, but I've had to fork a work project with this solution to get things back to work. Let me know what you think!

@alexkrolick
Copy link
Collaborator

Well... that seems like it would bloat the CommonJS module a lot.

Maybe we should switch to ES modules and bundle with Rollup...

@alexkrolick
Copy link
Collaborator

alexkrolick commented Jun 27, 2018

@one19 Check out #375

Ultimately to prevent a breaking API change I added both module.exports.default and module.exports, like your PR proposes. However by adding ES module support in theory some use cases will be more optimized.

@one19
Copy link
Author

one19 commented Jun 27, 2018

Wow! Thanks for replying so hyperfast!

I'll respond with the results of my investigation into using the beta version in that PR and close this one!

@one19 one19 closed this Jun 27, 2018
@alexkrolick alexkrolick mentioned this pull request Jun 29, 2018
@alexkrolick
Copy link
Collaborator

alexkrolick commented Jul 3, 2018

Actually, the way I went about adding module.exports doesn't work because you can't mix ES imports with CJS exports. May have to merge this as 1.4.0 and release the other PR as 2.0.0 with module.exports deprecated.

alexkrolick added a commit that referenced this pull request Jul 7, 2018
alexkrolick added a commit that referenced this pull request Jul 7, 2018
@alexkrolick
Copy link
Collaborator

@one19 merged as #384

Can you try #375 (1.4.0-beta.5 or @next) with TS as well? I'm trying to avoid a 2.0 just for a build change.

@one19
Copy link
Author

one19 commented Jul 11, 2018

1.4.0-beta.5 worked! Thanks!

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

2 participants