Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[minor] fix: recursive issue with Draft and JSONTypes #990

Merged
merged 1 commit into from
Jan 15, 2023

Conversation

unional
Copy link
Contributor

@unional unional commented Oct 30, 2022

fixes #839

I have added test under __tests__/types/type-externals.ts.

There are several things I have observed:

  • the default formatter setting in vscode is not set to prettier. I have changed that so I won't mess up your formatting).
  • there are some test failure from the previous PR
  • the project doesn't build in Windows: yarn build -> 'cp' is not recognized as an internal or external command
  • there are some circular dependency issues:
Circular dependency: src\internal.ts -> src\types\types-internal.ts -> src\internal.ts
Circular dependency: src\plugins\patches.ts -> src\immer.ts -> src\plugins\all.ts -> src\plugins\patches.ts
Circular dependency: src\internal.ts -> src\types\types-internal.ts -> src\internal.ts
Circular dependency: src\internal.ts -> src\utils\common.ts -> src\internal.ts
Circular dependency: src\internal.ts -> src\utils\plugins.ts -> src\internal.ts
Circular dependency: src\internal.ts -> src\core\scope.ts -> src\internal.ts
Circular dependency: src\internal.ts -> src\core\finalize.ts -> src\internal.ts
Circular dependency: src\internal.ts -> src\core\proxy.ts -> src\internal.ts
Circular dependency: src\internal.ts -> src\core\immerClass.ts -> src\internal.ts
Circular dependency: src\internal.ts -> src\core\current.ts -> src\internal.ts
Circular dependency: src\plugins\patches.ts -> src\immer.ts -> src\plugins\patches.ts
Circular dependency: src\plugins\patches.ts -> src\immer.ts -> src\plugins\all.ts -> src\plugins\patches.ts
Circular dependency: src\internal.ts -> src\types\types-internal.ts -> src\internal.ts
Circular dependency: src\internal.ts -> src\utils\common.ts -> src\internal.ts
Circular dependency: src\internal.ts -> src\utils\plugins.ts -> src\internal.ts
Circular dependency: src\internal.ts -> src\core\scope.ts -> src\internal.ts
Circular dependency: src\internal.ts -> src\core\finalize.ts -> src\internal.ts
Circular dependency: src\internal.ts -> src\core\proxy.ts -> src\internal.ts
Circular dependency: src\internal.ts -> src\core\immerClass.ts -> src\internal.ts
Circular dependency: src\internal.ts -> src\core\current.ts -> src\internal.ts
Circular dependency: src\plugins\patches.ts -> src\immer.ts -> src\plugins\patches.ts

I think these should be fixed.

Here is an video about the details: https://www.youtube.com/watch?v=PCRzWIubAEQ

@netlify
Copy link

netlify bot commented Oct 30, 2022

Deploy Preview for quizzical-lovelace-dcbd6a canceled.

Name Link
🔨 Latest commit 7e2c3d1
🔍 Latest deploy log https://app.netlify.com/sites/quizzical-lovelace-dcbd6a/deploys/635ee39eb6a5b700082b76fe

Copy link
Collaborator

@mweststrate mweststrate left a comment

Choose a reason for hiding this comment

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

Looking good, will merge!

@mweststrate mweststrate changed the title fix: recursive issue with Draft and JSONTypes [minor] fix: recursive issue with Draft and JSONTypes Jan 15, 2023
@mweststrate mweststrate merged commit b9eae1d into immerjs:master Jan 15, 2023
@elijahbenizzy
Copy link

Nice work! Thank you!

@github-actions
Copy link
Contributor

🎉 This PR is included in version 9.0.18 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Methuselah96
Copy link
Contributor

@mweststrate This PR does not properly fix the issue and introduces bugs in other scenarios (#1013). It should probably be reverted.

Methuselah96 added a commit to Methuselah96/immer that referenced this pull request Jan 16, 2023
Methuselah96 added a commit to Methuselah96/immer that referenced this pull request Jan 16, 2023
mweststrate pushed a commit that referenced this pull request Jan 16, 2023
@mweststrate
Copy link
Collaborator

Note that this PR was reverted, see #1013, #1014

@unional
Copy link
Contributor Author

unional commented Jan 16, 2023

Thanks for reverting. The fix didn't take account for union type. Will see if I can make a proper fix for that later. Can we reopen #839?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeScript - Recursive types: Type instantiation is excessively deep and possibly infinite - ts(2589)
4 participants