-
Notifications
You must be signed in to change notification settings - Fork 822
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
Make Function Env immutable #1663
Conversation
Work in progress, we still have to update Emscripten
bors try |
tryBuild failed: |
bors try- |
bors try |
tryBuild failed: |
I agree |
bors try |
tryBuild failed: |
bors try |
tryBuild failed: |
bors try- |
bors try |
# enables internal features used by the deprecated API. | ||
deprecated = [] |
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.
Optional: really wish we didn't have to do this. Deprecated ought to be built on top of the API. Or put another way, anything deprecated does should be supported by our real API as something that other library consumer may also want to do.
Not that I'm offering an alternative. IIUC we considered wrapping it in a Mutex and rejected that?
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.
Not that I'm offering an alternative. IIUC we considered wrapping it in a Mutex and rejected that?
That was a different PR, if it were as simple as just conditionally putting something behind a mutex, then I agree. I tried 2-3 times to make it work without editing the API and this is the only way I've been able to make it work.
I agree that it kind of sucks, but I figure the deprecated API won't be around forever so 🤷
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.
IMO given how all the parts deprecated needs are now unsafe
and documented as non-stable, you could remove the deprecated
feature. A pro of this is that we don't have two possible builds of lib/api, with deprecated enabled and disabled. Maybe that doesn't matter.
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.
Looks great to merge, once the "deprecated" comment from Nick is addressed :)
bors r+ |
Alternative / related to #1625
Resolves #1612
Review