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

feat: add express example #2306

Merged
merged 1 commit into from
Feb 28, 2019
Merged

feat: add express example #2306

merged 1 commit into from
Feb 28, 2019

Conversation

nabdelgadir
Copy link
Contributor

@nabdelgadir nabdelgadir commented Jan 29, 2019

Closes #1982.

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

@nabdelgadir
Copy link
Contributor Author

To be able to use express.index.ts, I'd need to modify examples/todo/dist/src/index.d.ts because https://github.com/strongloop/loopback-next/blob/master/examples/todo/index.js#L6 which means application.main needs to use index.ts as dist regenerates so I can't add express.index.ts's main function there.

Another option would be to add the code to index.ts and comment out the express parts (but commented code wouldn't look nice). I can also not add the code to the example at all and keep it simply in the tutorial doc, but then I wouldn't be able to add tests.

I also tried creating a new class called ExpressApplication which would be something like:

import * as exp from 'express';
const express = require('express');

export class ExpressApplication {
  // doesn't recognize express unless it's imported
  app: exp.Application;
  constructor() {
    this.app = new express();
  }
}

But then calling const app = new ExpressApplication(); from main and using it would result in the same commented code.

Any thoughts?

@nabdelgadir nabdelgadir self-assigned this Jan 30, 2019
@dhmlau
Copy link
Member

dhmlau commented Jan 30, 2019

I wonder if it would be better to separate this from the todo tutorial. IIUC, the todo tutorial is meant to be the very first tutorial someone would follow. It should contain the minimal set of instruction in order to get things started. If we keep adding customized/advanced stuff in it, I think it defeats the purpose of the todo tutorial.

@nabdelgadir
Copy link
Contributor Author

nabdelgadir commented Jan 30, 2019

@dhmlau yes, I agree. Originally I wanted to make a separate tutorial, but there's only:

const app = new express();
const lbApp = new TodoListApplication(options);

// Mount LB4 as our REST API
await lbApp.boot();
lbApp.basePath('/api');

// Expose the front-end assets via Express, not as LB4 route
app.use(lbApp.requestHandler);
app.use(express.static('./public'));

// Add any custom Express routes
app.get('/', function(_req: Request, res: Response) {
  res.sendFile(path.resolve('public/index.html'));
});
app.get('/get', function(_req: Request, res: Response) {
  res.send('Get!');
});

app.listen(3000);

to add. That's why I wasn't sure if it warranted its own tutorial, since it would be really simple.

Alternatively, what about adding it as a "How to"?

@dhmlau
Copy link
Member

dhmlau commented Jan 30, 2019

What we can do is to have a page under How to section, doesn't need to be part of the todo app code. Just a thought.

@nabdelgadir
Copy link
Contributor Author

@dhmlau that's also what I was considering. I think that's the approach that I will go with, thanks!

@raymondfeng
Copy link
Contributor

At high level, I think it's better to have a separate package for the express example, such as examples/express-composition. The todo application can be a dependency of the new module and imported into the express app.

@nabdelgadir
Copy link
Contributor Author

@raymondfeng on second thought, that might be a better approach than just adding it to the How to section because, although it would be a simple app, then I can at least add tests.

@jannyHou
Copy link
Contributor

I think it's better to have a separate package for the express example, such as examples/express-composition.

+1 for creating a new example application, the app could be simpler than the todo app, e.g. having a Note model attached to a memory datasource.

@bajtos
Copy link
Member

bajtos commented Jan 31, 2019

on second thought, that might be a better approach than just adding it to the How to section because, although it would be a simple app, then I can at least add tests.

Yes please, it's important to have automated tests in place that will verify LB4-mounted-in-Express setup. @nabdelgadir has already discovered a problem in this integration, there may be more tricky aspects we are not aware of now or that we may accidentally break in the future. Automated test cases will prevent such regressions.

While it's tempting to require existing todo application into the new Express project, I think such setup will make the example less useful to our users. I expect most of our users to have both LB4 artifacts and Express code inside the same package (project), thus our example should show the same.

I think it will be best to create a new example project (package) as suggested by Raymond, and use a simpler LB4 setup than we have in the Todo app, e.g. configure a dummy Note model as proposed by Janny.

@nabdelgadir nabdelgadir force-pushed the express-mounting-tutorial branch 3 times, most recently from 58d13e3 to f90cc95 Compare February 7, 2019 01:21
@nabdelgadir nabdelgadir changed the title [WIP] feat: add express to todo tutorial [WIP] feat: add express example Feb 7, 2019
@raymondfeng
Copy link
Contributor

@nabdelgadir We should not duplicate the Todo code here. This example should declare a dependency of @loopback/example-todo and import the app to be composed with the express part.

@nabdelgadir
Copy link
Contributor Author

@raymondfeng although the todo and note examples are similar, I assume the user won't import their own LB app as a dependency. From @bajtos:

While it's tempting to require existing todo application into the new Express project, I think such setup will make the example less useful to our users. I expect most of our users to have both LB4 artifacts and Express code inside the same package (project), thus our example should show the same.

@raymondfeng
Copy link
Contributor

While it's tempting to require existing todo application into the new Express project, I think such setup will make the example less useful to our users. I expect most of our users to have both LB4 artifacts and Express code inside the same package (project), thus our example should show the same.

I see two flavors:

  1. The composite express app has all local modules within the same project structure. Ideally, such layout is supported by lerna as a monorepo.

  2. The composite express app has dependencies to LB4 apps as modules from npm.

Personally, I think 2 is a good starting point so that we can decompose the Express (web) and LB4 (api).

@nabdelgadir
Copy link
Contributor Author

@raymondfeng I'm okay with either approach personally, but @bajtos what do you think?

@bajtos
Copy link
Member

bajtos commented Feb 11, 2019

I am afraid I don't fully understand the difference between the two flavors described by @raymondfeng in #2306 (comment).

As a LB4 user, I would like to see an example that will teach me how can I build an application that uses top-level Express for certain parts and LB4 for other parts (most notably the REST API layer) and show me a project layout I can follow in my own project.

I am concerned that an example based on package composition would be too complex for an average user, both to understand the concept while learning and to apply it to their own project.

The composite express app has all local modules within the same project structure. Ideally, such layout is supported by lerna as a monorepo.

To me, this is a good setup for advanced users. I don't think it's possible to setup a lerna monorepo (example project) within another monorepo (loopback-next). To provide a meaningful example, we should implement this variant as a standalone GitHub repo.

Unfortunately, that makes it difficult to test Express+LB4 integration within loopback-next.

The composite express app has dependencies to LB4 apps as modules from npm.

I find this overly complicated. In real world, it requires the users to setup private npm packages (e.g. a paid account on npmjs.org or their own private registry) and orchestrate a deployment pipeline that will deploy a new version of the top-level Express app when a new version of a dependent LB4 app is published to the registry.

I am proposing the following steps forward:

  • As part of this user story, create a new example project (package) that will create a top-level Express app, mount LB4 app with a single Note model, and keep all artifacts in the same npm package.
  • Create a follow-up story to create a more advanced example showing how to compose Express and LB4 components via monorepo layout.

@nabdelgadir nabdelgadir changed the title [WIP] feat: add express example feat: add express example Feb 18, 2019
@nabdelgadir nabdelgadir force-pushed the express-mounting-tutorial branch 5 times, most recently from 0e5d36a to 042fba1 Compare February 20, 2019 14:42
Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

@nabdelgadir Great stuff!

I followed the instructions and run npm start to launch the express-composition example, I might miss some of the discussion, while the link to explorer is not working, it directs to http://127.0.0.1:3000/explorer/ and saying Cannot GET /explorer/, could you check? Other paths are good 👍

@nabdelgadir nabdelgadir force-pushed the express-mounting-tutorial branch 2 times, most recently from 6b2291c to 64a0371 Compare February 25, 2019 13:59
@nabdelgadir nabdelgadir force-pushed the express-mounting-tutorial branch 4 times, most recently from ba9d74f to 1c4da48 Compare February 27, 2019 21:15
Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

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

Awesome tutorial. I've left some comments and questions, but LGTM overall 👍

docs/site/express-with-lb4-rest-tutorial.md Outdated Show resolved Hide resolved
docs/site/express-with-lb4-rest-tutorial.md Show resolved Hide resolved
docs/site/express-with-lb4-rest-tutorial.md Show resolved Hide resolved
docs/site/express-with-lb4-rest-tutorial.md Show resolved Hide resolved
docs/site/express-with-lb4-rest-tutorial.md Show resolved Hide resolved
docs/site/express-with-lb4-rest-tutorial.md Show resolved Hide resolved
examples/express-composition/src/migrate.ts Outdated Show resolved Hide resolved
@nabdelgadir nabdelgadir merged commit dd2400e into master Feb 28, 2019
@nabdelgadir nabdelgadir deleted the express-mounting-tutorial branch February 28, 2019 15:32
// For testing purposes
public async stop() {
if (!this.server) return;
this.server.close();
Copy link
Contributor

Choose a reason for hiding this comment

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

If we decide to start lbApp in start() above, then we should also stop it IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add this in a follow-up PR or just remove starting the LoopBack app depending on #2306 (comment)

}

public async start() {
await this.lbApp.start();
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, my previous comment was more of a question instead of a request. Do we have configuration changes in place to make sure that LoopBack and Express are listening on different ports? @raymondfeng @bajtos @strongloop/loopback-maintainers thoughts?


public async start() {
await this.lbApp.start();
this.server = this.app.listen(3000);
Copy link
Member

Choose a reason for hiding this comment

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

IMO, we should honor rest.port and rest.host provided via ApplicationConfig.

Without that, most of the configuration code in index.js is effectively ignored.

  // Run the application
  const config = {
    rest: {
      port: +process.env.PORT || 3000,
      host: process.env.HOST || 'localhost',
      openApiSpec: {
        // useful when used with OASGraph to locate your application
        setServersFromRequest: true,
      },
    },
  };

What's worse, the application cannot be deployed to http://12factor.net environment like https://www.heroku.com

/cc @nabdelgadir

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make a new PR for this, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants