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

Change CJS export to ESM #25

Open
liorp opened this issue May 8, 2022 · 12 comments · May be fixed by #26
Open

Change CJS export to ESM #25

liorp opened this issue May 8, 2022 · 12 comments · May be fixed by #26
Assignees
Labels
enhancement New feature or request

Comments

@liorp
Copy link

liorp commented May 8, 2022

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.

<Konami>
    <MyElement/>
</Konami>

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!

@vmarchesin
Copy link
Owner

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.

@liorp
Copy link
Author

liorp commented May 9, 2022 via email

@vmarchesin
Copy link
Owner

I've published a new version 2.3.0 that should fix this. You can test this by cloning this repository and trying it inside the example folder with a production build of next . If it doesn't work then it might be a problem caused by your configuration. I'll wait for your reply to close this issue.

@vmarchesin vmarchesin added the bug Something isn't working label May 9, 2022
@vmarchesin vmarchesin self-assigned this May 9, 2022
@liorp
Copy link
Author

liorp commented May 9, 2022

Nope, it still happens, even after updating
"react-konami-code": "^2.3.0"

@vmarchesin
Copy link
Owner

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.

"dependencies": {
  "next": "12.1.4",
  "react": "18.0.0",
  "react-dom": "18.0.0",
  "react-konami-code": "^2.3.0"
},
"devDependencies": {
  "@types/node": "17.0.23",
  "@types/react": "18.0.1",
  "@types/react-dom": "18.0.0",
  "typescript": "4.6.3"
}

I have a next/image as the child of Konami. I tried the production bundle using next build and it works just fine.

<Konami>
  <Image
    alt="Image"
    src={imgSrc}
    width={256}
    height={256}
    priority
  />
</Konami>

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?

@liorp
Copy link
Author

liorp commented May 9, 2022

Yes, I have an example project for you to check:
liorp/liorpdev@2cbbce5
Please note that I used the useKonami hook in the last commit to main, so you should look at the commit before (2cbbce554f025c6813c7b1276e6d04c814e8b1a0).
It has a dependency of "react-konami-code": "^2.2.2", but it also fails in 2.3.0 (I just checked it, but didn't commit and push to avoid down time to my site).
Thank you very much for your help on this.

@vmarchesin
Copy link
Owner

I believe this repo is private and I do not have access to it.

@liorp
Copy link
Author

liorp commented May 9, 2022 via email

@vmarchesin
Copy link
Owner

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:

  1. 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.
  2. Change your import to:
import K from 'react-konami-code'
// @ts-ignore
const Konami = K.default ? K.default : K
  1. 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 :)

@vmarchesin vmarchesin added wontfix This will not be worked on and removed bug Something isn't working labels May 9, 2022
@liorp
Copy link
Author

liorp commented May 9, 2022 via email

@vmarchesin vmarchesin added enhancement New feature or request and removed wontfix This will not be worked on labels May 9, 2022
@vmarchesin
Copy link
Owner

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 library.type to module as per Webpack docs.

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.

@vmarchesin vmarchesin changed the title Can't have children that are not text Change CJS export to ESM May 9, 2022
@liorp
Copy link
Author

liorp commented May 9, 2022

I hope to have some free time soon to actually work on the code itself.
In the meantime I strongly recommend you to look into tsup. I've used it to build a library of mine, and it is really fast and convenient.
Again, thanks for your work!

@liorp liorp linked a pull request May 21, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants