-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
0302da8
to
85f883d
Compare
To be able to use Another option would be to add the code to I also tried creating a new class called 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 Any thoughts? |
I wonder if it would be better to separate this from the todo tutorial. IIUC, the |
@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"? |
What we can do is to have a page under |
@dhmlau that's also what I was considering. I think that's the approach that I will go with, thanks! |
At high level, I think it's better to have a separate package for the express example, such as |
@raymondfeng on second thought, that might be a better approach than just adding it to the |
+1 for creating a new example application, the app could be simpler than the todo app, e.g. having a |
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 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. |
58d13e3
to
f90cc95
Compare
@nabdelgadir We should not duplicate the |
@raymondfeng although the
|
I see two flavors:
Personally, I think 2 is a good starting point so that we can decompose the Express (web) and LB4 (api). |
@raymondfeng I'm okay with either approach personally, but @bajtos what do you think? |
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.
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.
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:
|
af5a0fa
to
8950449
Compare
0e5d36a
to
042fba1
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.
@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 👍
6b2291c
to
64a0371
Compare
64a0371
to
1f991d7
Compare
ba9d74f
to
1c4da48
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.
Awesome tutorial. I've left some comments and questions, but LGTM overall 👍
1c4da48
to
4516949
Compare
// For testing purposes | ||
public async stop() { | ||
if (!this.server) return; | ||
this.server.close(); |
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.
If we decide to start lbApp
in start()
above, then we should also stop it IMO.
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.
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(); |
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.
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); |
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.
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
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.
I'll make a new PR for this, thanks!
Closes #1982.
Checklist
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated