-
Notifications
You must be signed in to change notification settings - Fork 148
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
fix: add typescript definition #122
Conversation
@adrum I finally had the time to review this PR and good job on this, the only concern I have is you have missed out on |
Is this PR still relevant? Seems like there's not so much more left to do before merging, or is |
@adrum @tasola Could either of you please review/go through this quickly? I am no expert in typescript, but I have tried to add as much typing as I could. Also I have incorporated this PRs content as well, this way the entire repository would be migrated to typescript, not just the public interface. Appreciate the contributions. Please refer to this PR #157 |
@privateOmega I am continuing on your PR on my fork to correctly implement a typescript approach, if you are okay with that. Wouldn't be perfect as first version, but we can move forward from then. I really would like to revitalize this package to modern standards! I already am able to correct emit the typescript files and the associated type definition files via rollup. Question: is there a reason to emit two different index files? The Sidenote: I have migrated to rollup 3.0 (will update to the newest version when the PR is ready). |
@Napokue Thats great news and I dont mind one bit, in fact I am happy that someone can contribute to this project while I cannot. Sounds good. ESM is for latest ecmascript module bundlers and UMD is a replacement for CJS and AMD, hence should work both in frontend and backend projects. Lets go with those for now. I was thinking of moving away from rollup to webpack, since I always had some issue or the other with rollup bundles, as evident by some issues raised in this project, but we can take it up as a different effort anyways, lets stick with only TS changes in here. Also I have rebased my PR and suggest for you also to do the same and lets get it all merged in soon. |
@privateOmega thank you for your response. At the moment I am very busy, but when I can find time I will continue with this. Rollup seems to be better for libraries than webpack as far as my research went, as it use tree shaking and remove unnecessary code that won't be used in runtime (not sure how and if that is doable in webpack). I have done quite a lot in my branch already outside only the TS changes, as those were needed to accommodate the TS changes. Like updating rollup to the newest version (that introduced breaking changes to rollup). Having said that, I was able to produce an ESM module that worked in both NodeJS and ReactJS. I will probably start over with what I was doing and try to minimize the changes, while still able to get to our first goal of the direction forward in this repository: support TS. I have to see where I left off, but maybe I can also split the changes and get the small changes in preparation for TS support in first. |
Unfortunately due to limited time I am not able to contribute to this at the moment. Hopefully someone else will take it upon them! |
You need to allow null argument values where appropriate in the export statement, e.g.: headerHTMLString: string | null |
I'm going to close this in favor of #157 . Converting to TypeScript is probably the best way to go in the long run. |
Resolves #79