-
Notifications
You must be signed in to change notification settings - Fork 180
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
Remove public api #858
Remove public api #858
Conversation
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.
Just for context - I believe the original intention behind the public API exposure was testability, but naturally it can be tested as a decoupled unit too, as you have demonstrated here. 👍🏻
I agree with the conservative decision of not exposing any API yet until we know what the consumer needs are. I reckon we'll investigate some use cases as part of #692
9376818
to
6d6c721
Compare
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.
Small suggestion to check for undefined, otherwise approved!
🚢 |
47bc023
to
e01b247
Compare
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
This PR depends on #857, which needs to be merged first.
Our
activate
function returnedmoduleCallers
andclientHandler
which enabled other extensions to use them. We didn't intend this behavior and don't want to expose any public API yet.See:
This PR removes the return value from
activate
and refactors theworkspaces.spec.ts
test, which used this API.