search

dance2die / calendar-dates

📆 Get a list of calendar dates with no external dependencies.
https://www.npmjs.com/package/calendar-dates
MIT License
14 stars 3 forks source link

Convert async API to a synchronous one #24

Closed ewolfe closed 5 years ago

ewolfe commented 6 years ago

I ran into the "issue" of this library being async while running jest snapshot testing.

This pull request refactors the API to be synchronous. It also updates all of the documentation, changelog, etc.

ewolfe commented 5 years ago

@dance2die any thoughts on this?

dance2die commented 5 years ago

Wow. I am sorry I've not checked this PR (buried in deluge of emails).

May I ask what the issue was with the test?

ewolfe commented 5 years ago

Sure. Jest snapshots aren't asynchronous (at least without getting clever), so when my tests were running the "return" value from await calendarDates.getDates(...) is undefined. Which isn't what I really wanted... I wanted the dates data.

This meant my React render method couldn't render my calendar, which meant my snapshot file was empty and wouldn't provide any confidence.

dance2die commented 5 years ago

You can test async code by marking the callback with async. Not sure why your snapshot testing would not return data.

I'd like to keep the API async and anyway we can make your snapshot testing work?

ewolfe commented 5 years ago

I can probably get my snapshot testing working. I'm running snapshots via storyshots and they have a guide here. I'm probably just going to use my "sync" fork though since it makes things a little more straight forward.

Maybe I'm missing something though. Does keeping calendar-dates async have any benefits over a synchronous approach?

dance2die commented 5 years ago

Synchronous will block the UI while async methods won't.

So if one has to show many months, sync methods will make the site look unresponsive.