refactor(cli): use options to specify port and OpenAPI server url #1900

marioestradarosa · 2018-10-23T00:59:41Z

This PR only applies the options object in the application to benefit the app developer in two areas:

For new developers change the application port number with ease
Avoid the step to specify the server url + port number when used by OASGraph (if not used, then I believe it won't hurt? )

This pull request supersedes #1705.

Checklist

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

raymondfeng · 2018-10-23T03:06:44Z

packages/cli/generators/app/templates/src/application.ts.ejs

+        rest: {
+          // your application listening port number, you can change this once
+          // in production
+          port: 3000,


Does it make sense now to have port: +process.env.PORT || 3000 now?

Yes, you are right, I forgot about the process.env.PORT support

is the plus symbol +process.env.PORT intentional ?

Yes. It converts the value to number.

raymondfeng · 2018-10-23T03:10:16Z

packages/cli/generators/app/templates/src/application.ts.ejs

+          openApiSpec: {
+            // useful when used with OASGraph to locate your application when
+            // resolving GraphQL schemas
+            servers: [{url: 'http://localhost:3000'}],


Is setServersFromRequest: true better as we don't always know the port in advance?

I have just placed setServersFromRequest: true and removed the servers: and it doesn't work, it can't find the REST api resolver. Also, I just removed the port number and it doesn't work. Only works whenever the full path like servers: [{url: 'http://localhost:3000'}] is specified.

thoughts?

Please elaborate on setServersFromRequest: true

If you have setServersFromRequest: true, what's servers from the generated openapi spec?

Please note that browsers may cache the response. I had to use private browsing sometimes to work around it.

If you have setServersFromRequest: true, what's servers from the generated openapi spec?

"servers": [ { "url": "http://[::1]:3010" } ]

However, the node_modules/oasgraph/cli.js http://localhost:3010/openapi.json still doesn't work. I guess the problem is with OASGraph not liking the format [::1] ?

Neither node_modules/.bin/oasgraph http://localhost:3000/openapi.json or probably the problem is the express-graphql since this is the one connecting the resolver from the GraphQL schema.

http://[::1]:3010 uses ipv6 address.

marioestradarosa · 2018-10-23T04:10:58Z

@raymondfeng setServersFromRequest: true works fine. I was querying the wrong schema from my browser. PTAL.

bajtos

Please see the discussion in #1705 on why it's a bad idea to apply process.env.PORT inside the Application class.

Accessing process.env deeply from our code is an anti-pattern I'd like to avoid.

For example, how are we going to test this in a safe way?

If we start npm test in an environment where process.env.PORT is accidentally configured (e.g. a local dev machine), tests checking the default port number would start failing.

I am proposing a different solution instead: Implement a new Booter that will parse commonly used PORT and HOST variables from the environment and apply them to REST server config accordingly.

As a simpler alternative, I proposed to apply environment-specific configuration in the top-level index.js file - I think that's the easiest way how to get this functionality implemented.

if (require.main === module) {
  // Run the application
  const config = {
    rest: {
      port: process.env.PORT, 
      host: process.env.HOST
    },
  };
  application.main(config).catch(err => {
    console.error('Cannot start the application.', err);
    process.exit(1);
  });
}

bajtos · 2018-10-23T14:56:21Z

packages/cli/generators/app/templates/src/application.ts.ejs

+        rest: {
+          // your application listening port number, you can change this once
+          // in production
+          port: process.env.PORT || 3000,


would you mind adding a host configuration too?

host: process.env.HOST || undefined

marioestradarosa · 2018-10-23T15:20:29Z

@bajtos, @raymondfeng PTAL

raymondfeng · 2018-10-23T15:54:16Z

packages/cli/test/integration/generators/app.integration.js

    assert.fileContent('src/application.ts', /constructor\(/);
    assert.fileContent('src/application.ts', /this.projectRoot = __dirname/);
+    assert.fileContent('src/application.ts', /openApiSpec: {/);
+    assert.fileContent('src/application.ts', /port: process.env.PORT || 3000/);


Is this still true after moving process.env.PORT to index.js?

You are right!, but it didn't give any error, it should have returned false, what would be happening?

do you think the || is being evaluated ?

Yeah, you regular expression might have wildcard matches.

marioestradarosa · 2018-10-23T17:17:13Z

@raymondfeng I addressed the test evaluation. PTAL

raymondfeng · 2018-10-23T17:56:25Z

packages/cli/generators/app/templates/index.js.ejs

-  application.main().catch(err => {
+  const config = {
+    rest: {
+      port: process.env.PORT,


+process.env.PORT || 0?

fixed!, thanks @raymondfeng

marioestradarosa · 2018-10-23T18:07:26Z

@bajtos PTAL

bajtos

LGTM.

bajtos · 2018-10-23T15:02:32Z

packages/cli/generators/app/templates/src/application.ts.ejs

+          port: process.env.PORT || 3000,
+          openApiSpec: {
+            // useful when used with OASGraph to locate your application
+            setServersFromRequest: true,


I think it makes sense to keep this part inside the Application class constructor 👍

Would it be difficult to modify the tests we are scaffolding by lb4 app and include a new check to verify that setServersFromRequest is enabled?

(OTOH, I still thinks it's a bug of oasgraph that they don't support relative server addresses.)

bajtos

Actually, I have to withdraw my previous approval. Please update all example projects to be consistent with the new index.js template. No further review is required for that.

bajtos · 2018-10-25T10:41:40Z

packages/cli/generators/app/templates/src/application.ts.ejs

 export class <%= project.applicationName %> extends BootMixin(RestApplication) {
 <% } -%>
  constructor(options: ApplicationConfig = {}) {
+    options = Object.assign(


Could you please verify that Object.assign is preserving deep values provided by options? I believe it will replace rest object as a whole, i.e. changing {rest: {port: 123, host: 'name'}} provided by the top-level index.js into {rest: {openApiSpec: {setServersFromRequest: true}}}. A possible solution is to use a deep-merge algorithm, there are many implementations available on npmjs. deepmerge seems like a popular one that's still actively maintained. lodash provides deep-merge or deep-assign API too.

Verified, it replaces it. I will take a look at deepmerge and lodash now.

Why not move the config to index.js.ejs?

That could also be a good solution @raymondfeng , simplifies everything for this context. @bajtos thoughts?

marioestradarosa · 2018-10-26T04:06:30Z

Please update all example projects to be consistent with the new index.js template

@bajtos / @raymondfeng could you check the new application.ts ?, and I also placed a default value for host on index.js

Now it works with lodash merge method and is preserving the port and host passed thru the index.js file. After that, I will update the examples too.

raymondfeng · 2018-10-26T04:28:24Z

I still prefer to move it into index.js.ejs. Using lodash to deep merge in application ctor is a bit overwhelming.

marioestradarosa · 2018-10-26T05:25:50Z

@raymondfeng I agree!, @bajtos are you ok with this PR now ?. If so, then I will change the examples also.

bajtos

👍

marioestradarosa requested review from bajtos, dhmlau and raymondfeng October 23, 2018 00:59

marioestradarosa mentioned this pull request Oct 23, 2018

docs: add tutorial to expose GraphQL APIs #1899

Merged

raymondfeng reviewed Oct 23, 2018

View reviewed changes

marioestradarosa force-pushed the add-openapi-server-property branch from 3c46b71 to 70b3fb7 Compare October 23, 2018 04:08

marioestradarosa force-pushed the add-openapi-server-property branch from 70b3fb7 to 8663413 Compare October 23, 2018 04:38

bajtos suggested changes Oct 23, 2018

View reviewed changes

marioestradarosa force-pushed the add-openapi-server-property branch from 8663413 to ecf130e Compare October 23, 2018 15:19

raymondfeng reviewed Oct 23, 2018

View reviewed changes

raymondfeng mentioned this pull request Oct 23, 2018

feat(rest): set port from env by default #1705

Closed

7 tasks

marioestradarosa force-pushed the add-openapi-server-property branch from ecf130e to 231d5b9 Compare October 23, 2018 16:17

raymondfeng approved these changes Oct 23, 2018

View reviewed changes

raymondfeng reviewed Oct 23, 2018

View reviewed changes

marioestradarosa force-pushed the add-openapi-server-property branch from 231d5b9 to 89838aa Compare October 23, 2018 18:06

raymondfeng approved these changes Oct 23, 2018

View reviewed changes

marioestradarosa force-pushed the add-openapi-server-property branch from 89838aa to 6a6f6b4 Compare October 25, 2018 02:47

bajtos approved these changes Oct 25, 2018

View reviewed changes

bajtos reviewed Oct 25, 2018

View reviewed changes

marioestradarosa force-pushed the add-openapi-server-property branch from 6a6f6b4 to 9107a26 Compare October 26, 2018 03:59

refactor(cli): use options to specify app port and openapi server

0f0a8a5

marioestradarosa force-pushed the add-openapi-server-property branch from 9107a26 to 0f0a8a5 Compare October 26, 2018 05:24

refactor: examples, specifies port,host and OpenAPI server

a89bb1e

marioestradarosa requested review from b-admike and hacksparrow as code owners October 26, 2018 06:03

bajtos approved these changes Oct 26, 2018

View reviewed changes

marioestradarosa merged commit 2e7d4bb into loopbackio:master Oct 26, 2018

marioestradarosa deleted the add-openapi-server-property branch October 26, 2018 13:01

bajtos mentioned this pull request Jan 31, 2019

[Spike] Deploy a basic LB4 application to IBM Cloud Kubernetes #1606

Closed

3 tasks

refactor(cli): use options to specify port and OpenAPI server url #1900

refactor(cli): use options to specify port and OpenAPI server url #1900

Uh oh!

Conversation

marioestradarosa commented Oct 23, 2018 • edited by bajtos Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marioestradarosa commented Oct 23, 2018

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marioestradarosa commented Oct 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marioestradarosa commented Oct 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marioestradarosa commented Oct 23, 2018

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marioestradarosa commented Oct 26, 2018

Uh oh!

raymondfeng commented Oct 26, 2018

Uh oh!

marioestradarosa commented Oct 26, 2018

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

marioestradarosa commented Oct 23, 2018 •

edited by bajtos

Loading