-
Notifications
You must be signed in to change notification settings - Fork 30
We have anymap at home #241
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
base: main
Are you sure you want to change the base?
Conversation
|
I know we talked about the originating problem being something related to wasm; did you ever figure out what was going on there? I admit I've been putting off this PR because I'm a little uncertain about reimplementing part of a crate to solve a problem like that. |
|
Oops sorry i linked the wrong issue. This is the right one #238 |
sanbox-irl
left a comment
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.
We should merge this -- it simply replaces a part of the API with an even smaller API that is trivial to maintain. That's just a win IMO!
I don't think this is a slam dunk for me. There are a couple tricks that anymap uses that this PR doesn't replicate that I like but also don't have any interest in maintaining. This PR makes sense to me if both:
|
|
but lucien, it would mean that i'd have one less anymap in my tree. think of the advantages! |
|
To be clear I don't think this should be merged as is! The PR is a sharp elbow in your side and also a place where you can tell me what should be done instead! |
What are those tricks? Just based on the PR, it doesn't feel like we actually use any thing not shown here |
Fixes #238