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

Issue #168 - Workaround for import errors when using typescript #382

Closed
wants to merge 2 commits into from
Closed

Conversation

aka-somix
Copy link

Summary

I investigated a little the Issue #168 because I am having the same problem and it is heavily slowing down my development.

It seems like the problem is mainly related to rollup.js and the decisions it makes while bundling the files.
My changes should be seen as a workaround more than a long term fix.

What I did change.

  1. It seems like if you have a single export in index.js, rollup.js will bundle it like this:
module.exports = Holidays.Holidays;

While by adding a second export, even if it is unused, it will correctly translate as:

exports.default = Holidays.Holidays;

which is what i think you expected to happen when the code was originally written (just like it happens in the date-holidays-parser library.

Thus, I added a "DumbClass" that does absolutely nothing but since it's imported it will force rollup.js to behave as expected.

  1. The second fix is actually about the date-holidays-parser library. In commonjs if you export something as:
exports.default = Holidays.Holidays;

Then it expects to be imported as

var HolidaysParser = require('date-holidays-parser').default;

instead of

var HolidaysParser = require('date-holidays-parser');

I could not figure out an elegant way of force rollup to do the import right, so i wrote a fast bash script that will do the postbuild and fix the import.

How did I test the changes.

I checked that the yarn test is still ok, so hopefully I did not introduced any breaking change for the JS users.

Then, I created a second project in typescript with the same configuration as the Issue #168 bug reporter, imported the library locally with npm and tested my changes.

Conclusion

Please, reach out to me if this PR is not complete. I did not had time to fully read the documentation but for what i understood I should have done every required step.

I know this is more of a workaround, but I need this to be fixed FAST as I am currently working with this library.

In the future, i would suggest to deprectate these default exports and create a new major version with module exports only, that are easier to manage with this whole bundle-up / typing stuff.
I would be happy to help with that!

@aka-somix aka-somix marked this pull request as ready for review December 4, 2022 23:27
@aka-somix aka-somix marked this pull request as draft December 5, 2022 16:08
@aka-somix aka-somix marked this pull request as ready for review December 5, 2022 16:08
@commenthol
Copy link
Owner

Hi @akaSomix,

Thanks for investigation the issue with typescript.
Hopefully you may understand that I can't accept this PR as this will break any implementation relying on javascript and not on typescript.

Let me explain:

The current export in index.js is:

module.exports = Holidays.Holidays;

you propose this as new "default" export:

exports["default"] = Holidays.Holidays;

Which simply means everyone has to require with your proposed .default.

I agree with you switching to named exports will solve this.
As I have never imagined the mess around switching to ESM while maintaining transpiled commonjs, I will definitely consider this.

Therefor my plans are to switch to solely ESM in the next major release and let everyone do the transpilation as they like.

@commenthol commenthol closed this Dec 10, 2022
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.

2 participants