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

Add Angular support #626

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

Conversation

ajguerrer
Copy link

@ajguerrer ajguerrer commented Sep 7, 2019

My first PR! I hope this PR provides a good first stab in closing #382. It adds a 'framework=angular' option which generates an Angular singleton service. Each method in the Angular service wraps the RPC in a rxjs observable, providing that distinctive Angular flavor and opening up use of a wide range of operators.

Metadata events, from on('status'), are not supported. Doing so requires generating code that, in my opinion, gets rather ugly. I decided to wait for an easier API, which seems to be in flight (looking at #603). I can share my on own thoughts on the API if interested.

An Angular 'echo' example app was added using Angular Material. Testing for the generated code was added the Angular ng test way. Let me know what you think.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 7, 2019

CLA Check
The committers are authorized under a signed CLA.

@@ -12,7 +12,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.

FROM node:8-stretch
FROM node:10-stretch
Copy link
Author

Choose a reason for hiding this comment

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

required for @angular/cli to work.

@stanley-cheung
Copy link
Collaborator

This is great! Thanks for the contributions. I will definitely take a look at this soon.

@ajguerrer
Copy link
Author

@stanley-cheung , Some design decisions here effect the API of the generator. I know such decisions require some foresight, so if something should be changed, I'm happy to do so.

Perhaps the most controversial decision is whether to include Angular here or make another protobuf plugin. Personally, I think moving Angular support to a separate plugin would make it 'second class'. I look forward to your thoughts on this.

@Dasio
Copy link

Dasio commented Nov 14, 2019

Is it possible to somehow support https://github.com/SafetyCulture/grpc-web-devtools?
I can't access client inside service because it's private.

@smnbbrv
Copy link

smnbbrv commented Nov 14, 2019

@Dasio I'm unsure if clients are not private its gonna be more helpful. Problem is you don't know when / whether the services are created.

The whole approach of the extension you pointed to is slightly weird. I filed there an issue SafetyCulture/grpc-web-devtools#54 which, if resolved, allows to have custom intercepters pushing the data in. This would be more helpful, however somebody still needs to add intercepters support on this side...

@kabriel
Copy link

kabriel commented Jan 2, 2020

Would love to see this merged

@ridewindx
Copy link

Would love to see this merged

@smnbbrv
Copy link

smnbbrv commented Feb 20, 2020

@ajguerrer
Copy link
Author

Nice work @smnbbrv! To be fair, ngx-grpc is probably the more practical solution. Judging by the time this PR has been open, I'm guessing there isn't enough bandwidth to maintain an Angular integration. I get the feeling it falls out of the scope of this project.

Making this PR was a fun way to learn more about grpc-web from the perspective of a hobbyist. If it helped someone then great, but I have no personal intention of seeing this merged unless the maintainers of this project have an interest in it. I thought maybe they did since its in the roadmap.

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.

6 participants