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.

Sign up for GitHub

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

Jump to bottom

Fix modify type inference #181

Merged
merged 3 commits into from
Apr 5, 2022
Merged

Fix modify type inference #181

merged 3 commits into from
Apr 5, 2022

Conversation

thewilkybarkid
Copy link
Contributor

When using modify to set a value (refs #165), I found the inference didn't work when there's a union type.

gcanti
gcanti requested changes Mar 31, 2022
View reviewed changes
docs/modules/Lens.ts.md Outdated Show resolved Hide resolved
@gcanti
Copy link
Owner

gcanti commented Mar 31, 2022

Should we change the signature of all modify / modifyOption APIs accordingly?

@thewilkybarkid
Copy link
Contributor Author

Should we change the signature of all modify / modifyOption APIs accordingly?

Only used the Lens one (so far) but I'll take a look.

@thewilkybarkid
Copy link
Contributor Author

@gcanti Hopefully that's them all.

I also took a look at set/setOption, but the change seems to create a type problem in prismAsOptional/prismAsTraversal that I don't understand yet so I've backed out of that for now.

@thewilkybarkid thewilkybarkid mentioned this pull request Mar 31, 2022
Open
@gcanti gcanti merged commit 57d857e into gcanti:master Apr 5, 2022
@gcanti
Copy link
Owner

gcanti commented Apr 5, 2022

Thanks @thewilkybarkid

@thewilkybarkid thewilkybarkid deleted the modify-type branch April 5, 2022 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gcanti gcanti gcanti approved these changes

@DenisFrezzato DenisFrezzato DenisFrezzato approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@thewilkybarkid @gcanti @DenisFrezzato