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

add monocole ts for all properties operations #8

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

regevbr
Copy link

@regevbr regevbr commented Jun 19, 2021

This PR replaces all the {...obj} usages in the code to use the awesome gcanti/monocle-ts library.
I couldn't help myself and I also updated some dependencies, fixed deprecated usages of fp-ts and transformed toFlags to be more "functional". I also made the CI run on PRs

@regevbr
Copy link
Author

regevbr commented Jun 19, 2021

@IMax153 can you please CR? all the tests are passing ofcourse

@IMax153
Copy link
Owner

IMax153 commented Jun 19, 2021

Hey @regevbr! Thank you so much for the PR - the lib is definitely well past due for an update, so this is much appreciated!

I’m on vacation for the next two weeks, but once I have a chance to sit down in front of my computer I will take a look and approve!

Comment on lines +37 to +38
"fp-ts": "^2.10.5",
"monocle-ts": "^2.3.10"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In thinking about this PR further, the addition of monocle-ts as a peer-dependency of expressive-ts would be considered a breaking change for users of the library. The update to the fp-ts version is also a soft breaking change, since it's a peer dependency of the lib.

I would like to avoid implementing any breaking changes until the [email protected] release, at which point we can collaborate on a revamp of the entire expressive-ts library.

We can leave this PR open until then if that is alright?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good

src/Expression.ts Outdated Show resolved Hide resolved
src/Expression.ts Outdated Show resolved Hide resolved
@regevbr
Copy link
Author

regevbr commented Jun 24, 2021

@IMax153 thanks for the CR! Can you please approve the workflow so the CI can run for this PR?

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