-
Notifications
You must be signed in to change notification settings - Fork 208
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: product recommendation #20
Conversation
Signed-off-by: virkt25 <[email protected]>
Signed-off-by: virkt25 <[email protected]>
|
||
import * as express from 'express'; | ||
import {Server} from 'http'; | ||
const recommednations = require('./recommendations.json'); |
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.
Typo: recommednations
=> recommendations
|
||
export const recommender = { | ||
start: () => { | ||
server = app.listen(3001, () => { |
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.
Let's make the module itself a factory function that takes a config object to start an instance, such as:
export function createRecommendationServer(config: ...) {}
src/services/recommender.service.ts
Outdated
import {Product} from '../models'; | ||
|
||
export interface RecommenderService { | ||
recommend(userId: string): Promise<Product[]>; |
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.
getProductRecommendations()
?
@@ -4,6 +4,7 @@ | |||
"include": [ | |||
"src", | |||
"test", | |||
"recommender", |
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.
At some point, we probably need to convert this repo to be lerna
managed monorepo.
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.
Yea probably ... eventually. I considered it but it seemed a bit excessive for a single file right now tbh.
Signed-off-by: virkt25 <[email protected]>
start: () => { | ||
server = app.listen(3001, () => { | ||
console.log( | ||
'Mock Product Recommender powered by Express started on port 3001', |
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.
Use ${port}
instead of hard-coded 3001.
|
||
return { | ||
start: () => { | ||
server = app.listen(3001, () => { |
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.
Use port
console.log( | ||
'Mock Product Recommender powered by Express started on port 3001', | ||
); | ||
}); |
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.
Should it return server
?
recommender.js
Outdated
'/recommender', | ||
)); | ||
|
||
module.exports = app.createRecommendationServer().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.
I think it should export app.createRecommendationServer()
instead so that it can be stopped`.
@@ -36,7 +36,9 @@ | |||
"posttest": "npm run lint", | |||
"test:dev": "lb-mocha --allow-console-logs DIST/test/**/*.js && npm run posttest", | |||
"prestart": "npm run build", | |||
"start": "node .", | |||
"start": "concurrently --kill-others \"npm run start:app\" \"npm run start:recommender\"", |
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.
Why not npm run start:recommender && npm run start:app
?
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.
Because then the start process hangs at npm run start:recommeder
as the server is running ad doesn't return a response to the command line to allow it to continue to the next npm
script.
We used concurrently
in https://github.com/strongloop/loopback4-example-microservices as well.
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.
Another option: https://www.npmjs.com/package/npm-run-all
import {Server} from 'http'; | ||
const recommendations = require('./recommendations.json'); | ||
|
||
export function createRecommendationServer(port: number = 3001) { |
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.
It should just take (options: http.ServerOptions = {port: 3001})
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.
No other options are configurable in the app and I can't find a http.ServerOptions
type.
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.
Oh, we only have https.ServerOptions
.
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.
Keeping it an options
object will allow us to configure it to be https too.
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.
So I should set the type to be https.ServerOptions
?
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.
No, https.ServerOptions is for cert/key.
Signed-off-by: virkt25 <[email protected]>
|
||
return app.listen(port, () => { | ||
console.log( | ||
'Mock Product Recommender powered by Express started on port 3001', |
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.
Please use `${port}.
Signed-off-by: virkt25 <[email protected]>
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.
LGTM, one minor comment.
}); | ||
|
||
it('returns product recommendations for a user', async () => { | ||
await client.get(`/users/userid/recommend`).expect(200, recommendations); |
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'm probably missing something, but shouldn't userid
be a number here?
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.
Mongo generated UUID for User model are strings
fixes loopbackio/loopback-next#1482
implements a mock recommendation service -- runs on port 3001.
implements a proxy service to talk to the recommendation service.
both services start by running
npm run start
.