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

Weak ref #15863

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Weak ref #15863

wants to merge 4 commits into from

Conversation

deltakosh
Copy link
Contributor

No description provided.

@deltakosh deltakosh enabled auto-merge (squash) November 20, 2024 19:51
@bjsplat
Copy link
Collaborator

bjsplat commented Nov 20, 2024

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

Copy link
Member

@RaananW RaananW left a comment

Choose a reason for hiding this comment

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

This can be considered a kind of a breaking change to be honest.

This forces people to change their typescript target or their lib (just like you had to do for our typescript build).
This is also the reason our build failed.

The suggestion I have will resolve the issue to all other users, because it will remove the reference to WeakRef on our public declarations. I know it changes the types, but it might prevent a few complaints on the forum.

packages/dev/core/src/Misc/observable.ts Outdated Show resolved Hide resolved
@deltakosh
Copy link
Contributor Author

What is your suggestion?

@RaananW
Copy link
Member

RaananW commented Nov 21, 2024

What is your suggestion?

it's in the comment - use any as the type that is used with the function that is exposed in our declarations. or do soemthing like:

type WeakRefBJS<T extends Function> = any;

and then use this in the _remove function.

This will force us to add the lib, which is fine, but will not expose the WeakRef usage.

@deltakosh
Copy link
Contributor Author

Makes sense! Thanks

@bjsplat
Copy link
Collaborator

bjsplat commented Nov 21, 2024

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@bjsplat
Copy link
Collaborator

bjsplat commented Nov 21, 2024

@bjsplat
Copy link
Collaborator

bjsplat commented Nov 21, 2024

@bjsplat
Copy link
Collaborator

bjsplat commented Nov 21, 2024

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.

5 participants