-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
chore: Add memoize util compat #31800
base: master
Are you sure you want to change the base?
chore: Add memoize util compat #31800
Conversation
📊 Bundle size report✅ No changes found |
packages/react-components/react-utilities-compat/library/README.md
Outdated
Show resolved
Hide resolved
packages/react-components/react-utilities-compat/library/README.md
Outdated
Show resolved
Hide resolved
packages/react-components/react-utilities-compat/library/README.md
Outdated
Show resolved
Hide resolved
packages/react-components/react-utilities-compat/library/README.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Makoto Morimoto <Humberto.Morimoto@microsoft.com>
packages/react-components/react-utilities-compat/library/tsconfig.json
Outdated
Show resolved
Hide resolved
packages/react-components/react-utilities-compat/library/README.md
Outdated
Show resolved
Hide resolved
|
||
/* eslint-disable @typescript-eslint/no-explicit-any */ | ||
|
||
declare class WeakMap { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will probably explode if this would reach public API surface, with tslib setup for compat packages this should be removed as WeakMap is part of ts definitions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Hotell I need to port utility functions with zero changes so that we can't introduce any bugs for partners. I don't expect it to ever be exported. Is the tslib WeakMap the exact same implementation? If not, then I think I need to leave this in here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand what's at play here but we might be forced to do some changes in order to not clash with v9 (not this case). I double-check the .d.ts
rolluped bundle emit and it is not exposed so we are good with this one.
is the tslib WeakMap the exact same implementation?
obviously it's not :D it's strongly typed with generics and other jazz
but because it's declared like this declare class
it would clash with existing typescript lib definitions
if it would be declared as interface global override ( allowed behaviour in TS ) it dangerous approach as it would affect whole application codebase , that's why I pointed this out.
hope it's more clear now.
packages/react-components/react-utilities-compat/library/src/memoize.ts
Outdated
Show resolved
Hide resolved
packages/react-components/react-utilities-compat/library/tsconfig.json
Outdated
Show resolved
Hide resolved
packages/react-components/react-utilities-compat/library/README.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feel free to resolve my last comment and ready to merge. ty
Added memoize utility functions for react-utilities-compat