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

1st Round Feedback Updates #6

Merged
merged 1 commit into from
May 1, 2019
Merged

Conversation

realModusOperandi
Copy link
Contributor

@realModusOperandi realModusOperandi commented Apr 26, 2019

Thanks for the review. I have a few responses to some of the points:

In this section, try to explain why user wants to use Angular to cosume a RESTful web service?

Hopefully I captured it succinctly. The AngularJS guide didn't really have any statements to this effect--perhaps whoever wrote it can give me a nudge in the right direction?

The app.component.html file shows up empty on the code column.

I set it to tags=** like the other files. Let me know if I should do something more/different here.

Followed the guide to the following point (working in the start directory), the http://localhost:9080/app URL gives "Context root not found" error message..

Unfortunately I'm not able to reproduce this. I did add a reminder/statement to build the frontend before accessing the URL if the service is already running, or to run the install liberty:start-server goals which should also build the frontend. If this persists, can you tell me your operating system and what sequence of maven commands you run in the course of following the guide?

There are lots of files under the src/main/frontend directory, but only the app.module.ts and app.component.ts files are being described in the guide. What are the other files used for? Can some of them be cleaned up?

This is one of Angular's drawbacks, I'm afraid. As far as I know, all of the files are at least expected, and the vast majority are required of any Angular application. I can add some explanation of why there are so many files if that would help, but users following this guide don't need to concern themselves with each one individually.

Part of my motivation for proposing this guide was to provide a sort of starting point for anyone interested in using Angular with OpenLiberty, so they wouldn't have to stumble through figuring out where to put all the files and how to configure the build--it would just work.

There are a number of files under src/main/webapp/app directory as well, and they're not being referred to in the guide. Are these files needed, and should be explained in the guide?

These files are the output of the Angular compiler and must be present at the root of the .war file in order for them to be served properly. I think these should be placed in the target directory but I am not a maven expert and was unable to find how to copy them to directly to the SNAPSHOT.war folder within target. I can investigate further if you'd like, but the folder is at least ignored in the .gitignore file.

(I corrected all the other minor changes requested. Good eye!)

@realModusOperandi realModusOperandi self-assigned this Apr 26, 2019
@evelinec evelinec self-requested a review April 29, 2019 18:43
@evelinec
Copy link
Contributor

Thanks a lot @realModusOperandi for the quick respond. I'll go through all the changes and your comments above. Thanks.

@evelinec
Copy link
Contributor

evelinec commented Apr 30, 2019

Thanks for the review. I have a few responses to some of the points:

In this section, try to explain why user wants to use Angular to consume a RESTful web service?

Hopefully I captured it succinctly. The AngularJS guide didn't really have any statements to this effect--perhaps whoever wrote it can give me a nudge in the right direction?

Yeah, we agreed the AngularJS guide did not have some of essential statements in the intro section. That was one of our first few guides; since then the guide’s content and requirements have gradually changed and improved. Take a look at few on our more recent guides below with references:

  1. Creating a RESTful web services: https://openliberty.io/guides/rest-intro.html#what-youll-learn
    Refer to paragraph: When you create a new REST application, the design of the API is important. The JAX-RS APIs could be used to create JSON-RPC, or XML-RPC APIs, but it wouldn’t be a RESTful service. A good RESTful service is designed around the resources that are exposed, and on how to create, read, update, and delete the resources.
  2. Injecting dependencies into microservices: https://openliberty.io/guides/cdi-intro.html#what-youll-learn
    Refer to paragraph: What is CDI? Contexts and Dependency Injection (CDI) defines a rich set of complementary services that improve the application structure. The most fundamental services that are provided by CDI are contexts that bind the lifecycle of stateful components to well-defined contexts, and dependency injection that is the ability to inject components into an application in a typesafe way. With CDI, the container does all the daunting work of instantiating dependencies, and controlling exactly when and how these components are instantiated and destroyed.

For the Angular guide, an additional explanation of why user want to use Angular to consume a RESTful web service, ie, what’s the advantage of using Angular, within the following paragraph or on a separate paragraph, would be great:
Angular is a framework for creating interactive web applications. Angular applications are written in HTML, CSS, and TypeScript, a variant of JavaScript. Angular applications must be compiled. The Angular compilation step has been configured as part of the Maven build.

The app.component.html file shows up empty on the code column.

I set it to tags=** like the other files. Let me know if I should do something more/different here.

I think replacing a tag that doesn’t exist should resolve this problem. I’ll test it on the staging site.

Followed the guide to the following point (working in the start directory), the http://localhost:9080/app URL gives "Context root not found" error message..

Unfortunately I'm not able to reproduce this. I did add a reminder/statement to build the frontend before accessing the URL if the service is already running, or to run the install liberty:start-server goals which should also build the frontend. If this persists, can you tell me your operating system and what sequence of maven commands you run in the course of following the guide?

I found that there was a port conflict issue on my system that’s why I got the “Context root not found” error. Since I cleaned my system, the application was able to start properly, and I can access the http://localhost:9080/app URL. After going through the guide in the start directory in the following steps, there’re still multiple issues, described along the steps.

  1. Run this git clone command to get the code: git clone https://github.com/openliberty/draft-guide-rest-client-angular.git --branch dev --single-branch
  2. Go to start directory
  3. Run mvn install liberty:start-server command
    When running the mvn install command for the first time, there’s this error message in the log: [ERROR] node-pre-gyp WARN Using request for node-pre-gyp https download
  4. Run mvn generate-resources command
  5. Perform this action: “Update the app.module.ts file. src/main/frontend/src/app/app.module.ts“
  6. Perform this action: “Update the app.component.ts file. src/main/frontend/src/app/app.component.ts“
  7. Perform this action: “Replace the template of your component. src/main/frontend/src/app/app.component.html“ where I got the code from the finish directory, as indicated in the readme.adoc.
  8. Then go the application URL as indicated in, “After everything is set up, point your browser to the web application root http://localhost:9080/app to see the following output:”
    At this time, I do not see the expected consumed result, but rather a “Welcome to the frontend!” page (image below).

image

  1. If at this time I run the mvn generate-resources command again, then refresh the application URL, then I see the expected output:
foo wrote 2 albums:
    Album titled album_one by foo contains 12 tracks
    Album tilted album_two by foo contains 15 tracks
bar wrote 1 albums:
    Album titled foo walks into a bar by bar contains 12 tracks
dj wrote 0 albums:

Environment:
System: MacOS v10.13.6
JDK: java version "1.8.0_151"
Java(TM) SE Runtime Environment (build 1.8.0_151-b12)
Java HotSpot(TM) 64-Bit Server VM (build 25.151-b12, mixed mode)

There are lots of files under the src/main/frontend directory, but only the app.module.ts and app.component.ts files are being described in the guide. What are the other files used for? Can some of them be cleaned up?

This is one of Angular's drawbacks, I'm afraid. As far as I know, all of the files are at least expected, and the vast majority are required of any Angular application. I can add some explanation of why there are so many files if that would help, but users following this guide don't need to concern themselves with each one individually.

Part of my motivation for proposing this guide was to provide a sort of starting point for anyone interested in using Angular with OpenLiberty, so they wouldn't have to stumble through figuring out where to put all the files and how to configure the build--it would just work.

Yes, it’d be helpful to add some explanation of why there are so many files, even if user don’t need to concern about these files, an overall explanation would be very helpful. You could state the advantage as you described here, ie, “ provide a sort of starting point for anyone interested in using Angular with OpenLiberty, so they wouldn't have to stumble through figuring out where to put all the files and how to configure the build--it would just work.”

There are a number of files under src/main/webapp/app directory as well, and they're not being referred to in the guide. Are these files needed, and should be explained in the guide?

These files are the output of the Angular compiler and must be present at the root of the .war file in order for them to be served properly. I think these should be placed in the target directory but I am not a maven expert and was unable to find how to copy them to directly to the SNAPSHOT.war folder within target. I can investigate further if you'd like, but the folder is at least ignored in the .gitignore file.

I'll look further about this as well, and get back to you then. Thank you.

(I corrected all the other minor changes requested. Good eye!)

Thank you!

@realModusOperandi
Copy link
Contributor Author

Thanks for looking this over again. I'll make the changes as you've described. In your steps going through the guide, you mention making changes to the .ts and .html files like the guide instructs and then refreshing the frontend and seeing no changes. This is expected--mvn generate-resources invokes the Angular compiler to generate the application, and only after that will changes reflect in the browser. Is there another place I should indicate that changes will only be picked up after a recompile of the frontend?

Also, the error about node-gyp is noise and, as far as I know, can be ignored. I can add a blockquote note to this effect if you'd like.

README.adoc Show resolved Hide resolved
README.adoc Show resolved Hide resolved
@evelinec
Copy link
Contributor

Thank you @realModusOperandi. The changes so far looks good, and the latest commit for adding the recompilation step for the frontend. I'll verify these from end-to-end once the PR is merged. Thanks.

@evelinec
Copy link
Contributor

Note just added couple minor points about the other changes in this PR.

@evelinec
Copy link
Contributor

Thanks for looking this over again. I'll make the changes as you've described. In your steps going through the guide, you mention making changes to the .ts and .html files like the guide instructs and then refreshing the frontend and seeing no changes. This is expected--mvn generate-resources invokes the Angular compiler to generate the application, and only after that will changes reflect in the browser. Is there another place I should indicate that changes will only be picked up after a recompile of the frontend?

I saw the changes you added for hte recompilation, it looks good. I'll try the guide again from end-to-end once we have the PR merged. Thank you.

Also, the error about node-gyp is noise and, as far as I know, can be ignored. I can add a blockquote note to this effect if you'd like.

About this error, I'll look further into it, and get back to you if anything. Thanks.

Copy link
Contributor

@evelinec evelinec left a comment

Choose a reason for hiding this comment

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

Will merge this PR to verify the guide on the test site.

@evelinec evelinec merged commit ef998e7 into OpenLiberty:dev May 1, 2019
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