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

Alternative to #1577 #1578

Closed
wants to merge 5 commits into from
Closed

Alternative to #1577 #1578

wants to merge 5 commits into from

Conversation

hhugo
Copy link
Member

@hhugo hhugo commented Mar 3, 2024

Cons: breaks 2/3 packages (ppx_expect, alcotest)
Pro: no override logic

@hhugo
Copy link
Member Author

hhugo commented Apr 11, 2024

@tkluck-janestreet, would you have time to look at this PR. The latest version no longer break compatibility with ppx_expect thanks to js Proxy. Can you confirm it works for you ?

@tkluck-janestreet
Copy link

TIL about Proxy. This genuinely looks like the best of all worlds @hhugo. I blocked some time for tomorrow for trying this out across the tests in our codebase.

@tkluck-janestreet
Copy link

Hi @hhugo it's looking good so far. I'm running it on our continuous integration and will report the results here on Monday. I'd be surprised if it's going to uncover any problems.

@tkluck-janestreet
Copy link

I was too optimistic; unfortunately using Symbols as keys to a WeakMap is a relatively new feature and it doesn't work on the version of node that I'm using.

Not saying I wouldn't be happy if you merged this. Do you have a policy about which javascript versions are supported?

@hhugo
Copy link
Member Author

hhugo commented Apr 15, 2024

I was too optimistic; unfortunately using Symbols as keys to a WeakMap is a relatively new feature and it doesn't work on the version of node that I'm using.

Not saying I wouldn't be happy if you merged this. Do you have a policy about which javascript versions are supported?

No policy, I just try to be reasonable.
So it seems that this doesn't work with node 18 which is still in maintenance mode until April 2025. I think it would be unfortunate to break such setup. Let's try something else, this PR was mostly a hack to avoid breaking 2 packages after all.

I think it's a better long term solution to break and fix ppx_expect and alcotest and come up with a better abstraction/api/helper inside jsoo.

@hhugo hhugo closed this Apr 15, 2024
@hhugo hhugo mentioned this pull request Apr 15, 2024
@hhugo hhugo deleted the fix-leak-chan branch December 4, 2024 21:51
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