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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request for advice upgrading post 6.12 #143

Closed
mesqueeb opened this issue Sep 12, 2020 · 16 comments
Closed

Request for advice upgrading post 6.12 #143

mesqueeb opened this issue Sep 12, 2020 · 16 comments
Labels
bug Something isn't working question More info is requested

Comments

@mesqueeb
Copy link
Contributor

mesqueeb commented Sep 12, 2020

馃 Question

Describe your question

I'm wondering if it's possible to make the types of O.Compact/O.MergeAll more beautiful in VSCode...

Ever since 6.12 + things kind changed a lot.

6.12

very beautiful:

image

6.15.5

image

latest 8.x

O.Compact was not deprecated in the source code, but is the same as 6.15.5:
image

Instead you suggested me to use O.MergeAll instead:
image

I would like your general advice on my library merge-anything which merges objects and did so with beautiful typing support (until 6.12).

  1. I'm not sure if I should force ts-toolbelt 6.12 or not and wanted to ask your general advice.

  2. If I do go for v8.x, should I do this as a "major" update?

  3. If I go with MergeAll, what should I tell my users in the release note about the new returned type of my merge function?

Search tags, topics

#_6.12 #upgrade #compact #mergeall

@millsp
Copy link
Owner

millsp commented Sep 12, 2020

Hey @mesqueeb I think that you should Patch/PatchAll. Thanks for letting me know, I will deprecate Compact.

@millsp millsp added the question More info is requested label Sep 12, 2020
@millsp
Copy link
Owner

millsp commented Sep 12, 2020

Actually I'm seeing that none of the deprecated things have been removed 馃く...

@millsp millsp added the bug Something isn't working label Sep 12, 2020
@millsp
Copy link
Owner

millsp commented Sep 12, 2020

So this is what changed (on the left <= v6, on the right > v6):

Merge -> Patch
MergeUp -> Merge
Compact -> PatchAll

So you should use PatchAll. I can't remember why but you couldn't use MergeAll (which would have the best type safety), I think that you had something like {[K: string]: any} that prevented that.

The reason behind the renaming of the tools is that Merge on the v6 merged without handling unions and optional fields - not like a JS merging. In the meantime MergeUp got so efficient overtime that I decided that this was actually THE real merge. But maybe people did not wan't a merge that handles optional fields, after all? Then from that, I created Patch, that is exactly like Merge (v8) but it does not handle optional fields - it patches if the fields don't exist.

@millsp millsp closed this as completed Sep 12, 2020
@mesqueeb
Copy link
Contributor Author

@millsp
Unfortunately, I just checked and it's the same for PatchAll.
I'm talking about how these types look in VSCode pop up.

O.Compact v6.12 is only way to show a merged type way more beautiful as-is without anything the dev might not understand. See difference:

O.PatchAll

image

Vs. O.Compact pre 6.12

image

So as you can see, this part looks weird and might confuse devs:
image
While this same part with v6.12 was beautiful:
image

@millsp millsp reopened this Sep 13, 2020
@millsp
Copy link
Owner

millsp commented Sep 13, 2020

Ok, that's just a display problem. In short, this is for handling circular references. If you explicitly want to display it neatly, you should combine it with Compute. I'll see in the future what I can do to improve this without introducing performance drops.

const starter = { name: 'Squirtle', types: { water: true } };
const newvals = { name: 'Wartortle', types: { fight: true }, level: 16 };

type t = A.Compute<O.PatchAll<{}, [typeof starter, typeof newvals], 'deep'>>;

@millsp millsp closed this as completed Sep 13, 2020
@mesqueeb
Copy link
Contributor Author

mesqueeb commented Sep 14, 2020

Hey @millsp I have made the edits as per your advice. And it seemed like everything worked for me with beautiful types.

However, upon build now I got an error:

  Uncaught exception in test/mergeAndConcat.ts

  test/mergeAndConcat.ts(14,3): error TS2589: Type instantiation is excessively deep and possibly infinite.

  Uncaught exception in test/index.ts

  test/index.ts(22,3): error TS2589: Type instantiation is excessively deep and possibly infinite.

This is what my code looks like with your suggestion integrated as return type:

image

Here is the error in my tests:

image

Maybe you consider it unrelated, since the error happens in deepEqual ?

Do you have any advice for me?

Here is the repo and branch:
https://github.com/mesqueeb/merge-anything/tree/production

@mesqueeb
Copy link
Contributor Author

idea: provide a version of PatchAll / MergeAll that does not work with circular references?

my library doesn't support merging objects with circular references anyway, so I'd greatly appreciate the performance boost from a PatchAll / MergeAll that also doesn't (if there is a performance penalty that is ) : D

and it will probably fix this issue as well.

@millsp
Copy link
Owner

millsp commented Sep 14, 2020

I cloned your repo and the tests are passing. Maybe you should nuke your node_modules. And upgrade to ts ^4

@millsp millsp added works as intended It works this way question More info is requested and removed question More info is requested bug Something isn't working labels Sep 14, 2020
@mesqueeb
Copy link
Contributor Author

@millsp I just tried updating all dependencies, but no luck.
Maybe I forgot to mention but: the issue is on the production branch. It's not the default branch. 馃槄

image

@millsp millsp reopened this Sep 14, 2020
@millsp millsp added bug Something isn't working and removed works as intended It works this way labels Sep 14, 2020
@millsp
Copy link
Owner

millsp commented Sep 14, 2020

Got a fix on the way!

@millsp
Copy link
Owner

millsp commented Sep 14, 2020

You can just use it without A.Compute now. Let me know if it works for you.

@millsp
Copy link
Owner

millsp commented Sep 15, 2020

@mesqueeb did it solve your problem?

@millsp
Copy link
Owner

millsp commented Sep 15, 2020

Actually, it could be nice if you can hold on. I'm planning to release a more generic version of Merge and Patch that will handle arrays and tuples perfectly.

@mesqueeb
Copy link
Contributor Author

@millsp I'm sorry I didn't check this thread past two days!

all problems are solved since v8.0.6
I don't even need the A.Compute any more, it shows beautifully by default.

PatchAll, MergeAll both work.

I ended up going for O.Assign because I realised that that's the correct one for me now!

@mesqueeb
Copy link
Contributor Author

@millsp PS, I'm happy to test anything else for you, I don't do array or tuple merging in any of my libraries though!
But let me know how I can help! : )

@millsp
Copy link
Owner

millsp commented Sep 17, 2020

ts-toolbelt@9 will be out in November, it will have breaking changes on Merge and Patch.

eliflores added a commit to serlo/api.serlo.org that referenced this issue Jul 4, 2023
As part of the update of `ramda` from 0.28.25 to 0.29.3, only the version of 9.6.0 `ts-toolbelt` is required (`ts-toolbelt` version 6.15.5 was removed from the dependencies). In the version 9.6.0  of `ts-toolbelt` `MergeUp` does not exist, and needs be replaced by `Merge`.

References:
* millsp/ts-toolbelt@7f38e59
* millsp/ts-toolbelt#143 (comment)
eliflores added a commit to serlo/api.serlo.org that referenced this issue Jul 4, 2023
As part of the update of `ramda` from 0.28.25 to 0.29.3, only the version of 9.6.0 `ts-toolbelt` is required (`ts-toolbelt` version 6.15.5 was removed from the dependencies). In the version 9.6.0  of `ts-toolbelt` `MergeUp` does not exist, and needs be replaced by `Merge`.

References:
* millsp/ts-toolbelt@7f38e59
* millsp/ts-toolbelt#143 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working question More info is requested
Projects
None yet
Development

No branches or pull requests

2 participants