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

expose general .lens(a => b, a => b => a) #269

Merged
merged 3 commits into from
Nov 1, 2022
Merged

expose general .lens(a => b, a => b => a) #269

merged 3 commits into from
Nov 1, 2022

Conversation

valyagolev
Copy link
Contributor

Fixes #268.

This is badly typed because I couldn't figure out OpticParams machinery just yet (see ignored type-errors in tests). Sending this PR as a WIP in case I received any suggestions or warnings.

@akheron
Copy link
Owner

akheron commented Oct 25, 2022

Thanks for the PR!

Instead of modifying src/index.ts you should add your changes to scripts/generate-index.ts for them to be correctly applied to all optic types. src/index.ts in generated by running scrips/generate-index.ts. (This limitation doesn't exist for the standalone optics, btw...)

You got pretty close to adding a lens constructor. The correct typings would look like this:

  lens<U>(
    view: (a: A) => U,
    update: (a: A, v: U) => A
  ): Lens<S, NextParams<T, Adapt<A, U>>, U>

This is very close to the generic iso. A limiting factor is that it's monomorphic, meaning that the parameter v of update must have the same type which view returns (U in this case). If you check what Adapt<A, U> HKT does, you see that it make sure that the type that is written must equal U, and in this case it produces the type A.

Building a polymorphic lens (or iso) would require more type juggling, and using it would also require the user to provide a HKT that tells what happens to the types upon a polymorphic write.

@valyagolev
Copy link
Contributor Author

@akheron Thank you for the help, I updated the PR. I'm confused with the .iso case though. Somehow the last tests, around .iso, do not type check. Perhaps I'm missing something

@akheron
Copy link
Owner

akheron commented Oct 27, 2022

You should remove all the polymorphic test cases since this lens is not polymorphic. You must pass the same type as the source when you set or modify. It would become more clear if the source type was just string, not string | number.

@valyagolev valyagolev changed the title WIP/proposal: expose general .lens(a => b, a => b => a) expose general .lens(a => b, a => b => a) Oct 27, 2022
@valyagolev
Copy link
Contributor Author

@akheron thank you for the explanation. Ready to merge?

@akheron akheron added the feature label Nov 1, 2022
@akheron akheron merged commit 78bccb1 into akheron:main Nov 1, 2022
@akheron
Copy link
Owner

akheron commented Nov 1, 2022

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

expose lens constructor?
2 participants