search

commenthol / date-holidays

worldwide holidays
https://commenthol.github.io/date-holidays/
Other
918 stars 238 forks source link

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

Closed aka-somix closed 1 year ago

aka-somix commented 1 year ago

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.

2) 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!

commenthol commented 1 year ago

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.