-
Notifications
You must be signed in to change notification settings - Fork 12
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
Change CJS export to ESM #25
Comments
Hey @liorp, could you give a little bit more information? Are you using TS or JS? I think I've found an issue, it looks like we missed the |
I’m using typescript indeed.
…Sent from my iPhone
On 9 May 2022, at 12:35, Vinicius Marchesin Araujo ***@***.***> wrote:
Hey @liorp, could you give a little bit more information? Are you using TS or JS? I think I've found an issue, it looks like we missed the children type in the types definition.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.
|
I've published a new version |
Nope, it still happens, even after updating |
Sorry but I'm unable to reproduce the error. I just tried it in a TypeScript project of my own running the following dependencies and my production bundle works.
I have a
All I'm saying is that I can't reproduce the issue on my side. Could you provide a snippet of the part of your code that doesn't work, or the full project if possible? |
Yes, I have an example project for you to check: |
I believe this repo is private and I do not have access to it. |
Changed to public. Sorry for that.
…Sent from my iPhone
On 9 May 2022, at 21:19, Vinicius Marchesin Araujo ***@***.***> wrote:
I believe this repo is private and I do not have access to it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.
|
Alright, I just checked and this is a Vite issue indeed :) To be specific it's rollup that doesn't like
You can fix this by doing either of these:
Changing the export to |
Cool, thanks for the thorough reply. I’ll try to get to exporting as esm. Do you mind if I opened a ticket tagged as improvement for this?
… On 9 May 2022, at 22:04, Vinicius Marchesin Araujo ***@***.***> wrote:
Alright, I just checked and this is a Vite issue indeed :) To be specific it's rollup that doesn't like cjs modules. For more information you can check this issue vitejs/vite#2139
react-konami-code is exported as a cjs module so this issue makes sense in this scenario. This is fixed by rollup/plugins#1165 but I don't know when it will be merged.
You can fix this by doing either of these:
Stop using Vite 😬 Although if you really want to use it there are other options. It looks like the fix in rollup is coming so this should be fixed soon anyway.
Change your import to:
import K from 'react-konami-code'
// @ts-ignore
const Konami = K.default ? K.default : K
Use the useKonami hook as you did.
Changing the export to esm is not a bad idea but I don't have any plans to do that now as the library is working just fine. If you want to submit a PR you're welcome to make the changes yourself and submit it for review (after ensuring it's working properly). If you have other questions feel free to ask, otherwise I'll close this issue soon :)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.
|
I'll edit the title of this issue so we keep the discussion here. I started some work in this branch here https://github.com/vmarchesin/react-konami-code/tree/esm-export but I'm not sure if I need to change the I'd have to look into that, and this seems like a a great opportunity for you so I'd hate to take that from you :D Feel free to open a PR if you manage to make it work. |
I hope to have some free time soon to actually work on the code itself. |
Hi there, great project!
While I was trying to use the package, I couldn't manage to get it to work with a custom element (not plain text), e.g.
I'm using Vite, and while it worked in development mode, it didn't work when I compiled to production, so it might be a problem with rollup.
Thanks!
The text was updated successfully, but these errors were encountered: