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

Adds test for purescript-concur-react #45 issue #5

Merged
merged 3 commits into from
Jun 10, 2020

Conversation

kamoii
Copy link

@kamoii kamoii commented Jun 8, 2020

This PR adds test for purescript-concur/purescript-concur-react#45.

  • Using purescript-spec for a testing framework.
    I don't have a particular preference for testing frameworks, but this one seemed to be the one used the most.
  • Separated the configuration for test as test.dhall since the package for testing had dependency on aff.
  • You can run test with npm run test or spago -x test.dhall test
  • Updated the package-set because js-timers package was not registered in the package-set.
    If it was a bad idea to update pacakge-set, can write the test without updating it.

@kamoii
Copy link
Author

kamoii commented Jun 9, 2020

I later realized that I may have misunderstood Observer's semantics.
I thought the continuation of Observer a -> Effect Unit would only be called once at most, but does the implementation of mkObserver mean that there are also Observer's that calls continuation multiple times?
If that's the case, then the Observer to Aff conversion I wrote is incorrect.

@ajnsit
Copy link
Member

ajnsit commented Jun 9, 2020

@kamoii thanks! This is interesting! However I wouldn't build this off the callbacks branch right now, it's experimental and might change drastically. This can build on the Aff version for now, and we can port it to callbacks as needed.

@kamoii kamoii changed the base branch from callbacks to master June 9, 2020 14:08
@kamoii
Copy link
Author

kamoii commented Jun 9, 2020

@ajnsit Rewrote the PR to base on master.

@ajnsit
Copy link
Member

ajnsit commented Jun 10, 2020

Looks good! Thanks!

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