search

andreypopp / validated

Validate your configurations with precise error messages
91 stars 14 forks source link

Fix type error with flow > 0.70 #30

Closed forticulous closed 5 years ago

forticulous commented 6 years ago

This addresses issue #29

It looks like there was a problem with the AnyNode variable named any. It's declared as the existential type and is used in the different node builder functions as the default value. But even if the type isn't declared it can only have one type so it can't work for all the node builder functions. I replaced the usage of the same variable with a new AnyNode and that seemed to resolve the issue

pronebird commented 6 years ago

How can one use any as a default value at all if that's the Flow keyword? 😮 I hope (given the patch is correct) that this will be merged soon, since this issue holds us from updating to Flow 0.70+.

andreypopp commented 6 years ago

Sorry, don't have time to maintain this package. I'd glad to add more collaborators to the repo.

Alxandr commented 5 years ago

@andreypopp can you add me as a collaborator? I might not be able to do much dev on it, but responding to issues and PRs I can manage. I would like to accept this fix and get a new version published, because it's currently blocking me.

Alxandr commented 5 years ago

I've published a working version. For now, you can either change to it or do as I've done, and set your version like this: "validated": "npm:@alxandr/validated@2.0.0". Works with latest flow version.

I made a major version bump, not because the API has changed, but because of flow updates, there might be new flow errors in your project with the update (ie. flow got better basically).

pronebird commented 5 years ago

@andreypopp could you please merge this or consider adding @Alxandr to collaborators.

andreypopp commented 5 years ago

Added @Alxandr as a collaborator.

pronebird commented 5 years ago

Thank you @andreypopp.

@Alxandr I've installed your fork and all warnings are gone! Now given you're a collaborator on this project, could you please merge this PR to master and tag the next release so we could keep using the official repo?

andreypopp commented 5 years ago

@Alxandr I need your npm handle as well to add on npm.

Alxandr commented 5 years ago

@andreypopp it's the same as on GitHub. https://www.npmjs.com/~alxandr.

I'm on a work trip atm, which ends tomorrow, so I'll likely not be able to merge anything in before Friday unfortunately. Writing this on my phone.

andreypopp commented 5 years ago

Done.

andreypopp commented 5 years ago

I've published 1.3.0.

But it errors with flow 0.77 :(

I'd suggest moving to using ReasonML/OCaml for those who want type safety + fast iteration speed. This is what I did.

pronebird commented 5 years ago

@andreypopp thanks for releasing 1.3.0!

I feel you man, I had better experience with TypeScript than Flow. But the project we have is knee-deep in Flow so it's not something we can replace in a day.

I am not sure what errors you see on Flow 0.77, it works very well for me. You mean tests fail in validated?

andreypopp commented 5 years ago

@pronebird yes, you can clone the repo, upgrade flow to 0.77 and run it.

Alxandr commented 5 years ago

@andreypopp the branch I released works with newer versions

andreypopp commented 5 years ago

@Alxandr that’s cool, did you identify the root issue of such breakages?

Alxandr commented 5 years ago

I fixed multiple things. First off, * has been deprecated. Also, I added explicit typings to some things I think. It's been a few weeks, and I only on a phone right now, but it's all public, so you can check out the commits over at my fork if you'd like.

andreypopp commented 5 years ago

Thank you.