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

added support to cjs #143

Closed
wants to merge 2 commits into from
Closed

added support to cjs #143

wants to merge 2 commits into from

Conversation

kvnglvz
Copy link

@kvnglvz kvnglvz commented Nov 29, 2023

Added support for CJS.

I just followed this tutorial on how to do it with rollup.
https://stackoverflow.com/questions/74937600/how-to-support-es-modules-and-commonjs-modules-at-the-same-time

Note: This is my very first contribution ever, please tell me if something is wrong with my PR. Thank you.

@adrienjoly
Copy link
Owner

Thanks for your contribution.

I'm a bit skeptical on what practical improvements these changes bring for users of this lib. Can you clarify what motivated you to make these changes?

@kvnglvz
Copy link
Author

kvnglvz commented Dec 1, 2023

This is related to this issue #133
It makes this package compatible for projects still using commonjs

@adrienjoly
Copy link
Owner

Thank for your reply! => I notified the people concerned by this issue so they can test it and help improve your PR before I can find more time to help.

On the top of my head, it would be great if this PR changed fewer things, I.e. change only the things that need to be in order to fix that specific problem. E.g. do we need to update so many dependencies for this to work?

Also, it seems that you duplicated the code of index.js into index.cjs. This is not ideal because every future change would need to be replicated in both files, which is easy to forget and could results in bugs affecting the JS or CJS integration only. => is there any way to reduce that duplication?

@adrienjoly
Copy link
Owner

adrienjoly commented Dec 1, 2023

Note: if the index.cjs file is generated by a npm script, we may want to make it run automatically for every new version of pdfreader => call that command from the release script (.github/workflows/nodejs.yml) and don't necessarily keep the generated file in the git repo.

@kvnglvz kvnglvz closed this Dec 6, 2023
@treeslikemedia
Copy link

I have a commonJS project i was hoping to use this in. Any reason this never got merged / still isn't supported?

@adrienjoly
Copy link
Owner

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