Skip to content

Use EffectFnX functions internally #28

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

Closed
hdgarrood opened this issue Mar 5, 2016 · 9 comments · Fixed by #70
Closed

Use EffectFnX functions internally #28

hdgarrood opened this issue Mar 5, 2016 · 9 comments · Fixed by #70
Labels
good first issue First-time contributors who are looking to help should work on these issues. status: accepted This issue is now ready to be implemented via a PR. type: housekeeping Repo-related things (e.g. fixing CI) that need to be done.

Comments

@hdgarrood
Copy link
Member

To prevent issues like #27 from occurring again.

@paf31
Copy link
Contributor

paf31 commented Mar 7, 2016

👍

@JordanMartinez JordanMartinez added the status: needs more info This issue needs more info before any action can be done. label Dec 7, 2021
@artemisSystem
Copy link
Contributor

From what i can tell, this issue is about using (what's now called) Effect.Uncurried (EffectFnX) instead of FnX and mkEffect. The use of mkEffect prevents things like #27 by my understanding, but it's still an old and convoluted way of doing things. Perhaps we could switch over to EffectFnX at the same time as changes for #19 are made.

@JordanMartinez
Copy link
Contributor

I was thinking the same thing when I reviewed your other PR. We should be using EffectFnX now.

@JordanMartinez JordanMartinez changed the title use eff-functions Use EffectFnX functions May 20, 2022
@JordanMartinez JordanMartinez added the type: breaking change A change that requires a major version bump. label May 20, 2022
@hdgarrood
Copy link
Member Author

My intention when I created this was only to change the implementations internally, not to change the type signatures of any exported functions - I don't think we should use EffectFn in the exported functions in this library.

@JordanMartinez JordanMartinez added good first issue First-time contributors who are looking to help should work on these issues. status: accepted This issue is now ready to be implemented via a PR. type: housekeeping Repo-related things (e.g. fixing CI) that need to be done. and removed status: needs more info This issue needs more info before any action can be done. type: breaking change A change that requires a major version bump. labels May 20, 2022
@JordanMartinez JordanMartinez changed the title Use EffectFnX functions Use EffectFnX functions internally May 20, 2022
@artemisSystem
Copy link
Contributor

Agreed

@colinwahl
Copy link
Contributor

colinwahl commented Oct 18, 2022

I ran into a case where the current unsafe mkEffect util causes a runtime error when running this code through purescript-backend-optimizer. There is some discussion on why: aristanetworks/purescript-backend-optimizer#29

I'd be happy to make this change if no one else is working on it at the moment.

@JordanMartinez
Copy link
Contributor

JordanMartinez commented Oct 18, 2022

I'm in agreement with making this breaking change to this library outside of the normal ecosystem breakage. With the advent of purs-backend-es, some things can't be optimized without this.

@colinwahl Go for it!

@colinwahl colinwahl mentioned this issue Oct 18, 2022
2 tasks
@colinwahl
Copy link
Contributor

This shouldn't be a breaking change - none of the values whose types are changing are exported, and the exported values all retain their same types.

@JordanMartinez
Copy link
Contributor

This shouldn't be a breaking change - none of the values whose types are changing are exported, and the exported values all retain their same types.

Oh nice! I wasn't aware of that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue First-time contributors who are looking to help should work on these issues. status: accepted This issue is now ready to be implemented via a PR. type: housekeeping Repo-related things (e.g. fixing CI) that need to be done.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants