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 type-level dictionaries in order to reduce the number of overload… #155

Merged
merged 20 commits into from
Jul 21, 2017

Conversation

gcanti
Copy link
Owner

@gcanti gcanti commented Jul 15, 2017

…ings

@SimonMeskens
Copy link
Collaborator

SimonMeskens commented Jul 15, 2017

The typechecker already reports a bug with the codebase I think (proof that it's needed? 😉). It complains that State<S, A> is not conform to HKT2<L, A>, because State uses _S instead of _L. I assume that we change that in State.ts?

@gcanti
Copy link
Owner Author

gcanti commented Jul 15, 2017

This is great, good catch!

Store wasn't compliant too

@gcanti
Copy link
Owner Author

gcanti commented Jul 15, 2017

I'm experimenting random errors in Applicative.ts and HKT.ts (I say random because my local build succeeded though travis and my vscode complain). Note the last two messages

error

@SimonMeskens
Copy link
Collaborator

Can you try something for me? In VSCode, press F1, type in "typescript", click "TypeScript: Select TypeScript Version" and click "Use Workspace Version". See if that fixes it in your VSCode. If it does, then that means Travis is using the incorrect version of TypeScript?

@SimonMeskens
Copy link
Collaborator

I can confirm that there are weird random errors popping up.

This one doesn't seem to give me the same issues, but I'm worried any attempt to typecheck the map will run into file ordering issues, which I assume is what's causing these heisenbugs
My bad, didn't realize linting was on, I'll check in the future
It's not a good idea to have a linter check frankencode like that anyway. Liable to break in the future if you do.
This one doesn't seem to give me the same issues, but I'm worried any attempt to typecheck the map will run into file ordering issues, which I assume is what's causing these heisenbugs
Linting

My bad, didn't realize linting was on, I'll check in the future
Disable linting for map checker

It's not a good idea to have a linter check frankencode like that anyway. Liable to break in the future if you do.
@SimonMeskens
Copy link
Collaborator

Sorry for the linting commits, I figured out how to run the linter now (didn't know there was one). It seemed to have done a weird nonsensical merge too. If it bothers you, I'll do a rebase at the end?

@SimonMeskens
Copy link
Collaborator

With the current method, you can't actually assign anything to the interface HKT<U, A> it seems, which might be bad. Then again, there really is no reason to have a HKT<U, A> anywhere in the code, as it's possible to convert to a concrete at compile time, so maybe this behavior is desirable.

Doesn’t work with ts 2.4.1. I must figure out what’s wrong or find
another solution
@gcanti
Copy link
Owner Author

gcanti commented Jul 16, 2017

@SimonMeskens no problem. It seems that this last version doesn't check wrong HKT2 phantom types anymore though

@SimonMeskens
Copy link
Collaborator

Yup, it currently strips out the check when compiling, but we could export the check, so that doesn't happen.

@gcanti
Copy link
Owner Author

gcanti commented Jul 16, 2017

I mean that if I change _L to _S in State (as it was before #155 (comment)) it doesn't raise any error

@SimonMeskens
Copy link
Collaborator

Fixed

@gcanti
Copy link
Owner Author

gcanti commented Jul 20, 2017

This branch also builds with [email protected] without any change.

@SimonMeskens if it's ok for you I'm going to merge this and publish a fp-ts@next release

@SimonMeskens
Copy link
Collaborator

I tried further reducing overloads, but I can't really find a way, so yeah, merge away

@gcanti gcanti merged commit 73c85be into master Jul 21, 2017
@gcanti gcanti deleted the overloadings-destroyer branch July 28, 2017 05:20
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