Updates Node Server for latest Node, Modules and Node-Redis v4 Client#99
Updates Node Server for latest Node, Modules and Node-Redis v4 Client#99simonprickett wants to merge 12 commits intoRediSearch:masterfrom
Conversation
![sonatype-lift[bot]](https://avatars.githubusercontent.com/in/9843?s=60&v=4){kind=small}
|
@chayim is there someone I should ping to look at this major overhaul of the node code for node-redis 4? Thanks! |
|
@chayim this one was a lot of work that substantially improves the Node codebase and it's sat unmerged for a significant time. Can we get this merged? |
|
Can we merge this @chayim ? |
|
@leibale can you have a look? @simonprickett can you resolve the README conflict? |
leibale
left a comment
leibale
left a comment
There was a problem hiding this comment.
Overall looks good, I left some comments.. :)
| @@ -1,4 +1,4 @@ | |||
| FROM node:10 | |||
| FROM node:16 | |||
| "scripts": { | ||
| "start": "node server.js", | ||
| "test": "echo \"Error: no test specified\" && exit 1" | ||
| "dev": "./node_modules/nodemon/bin/nodemon.js", |
There was a problem hiding this comment.
npm adds "./node_modules/.bin" to the path, "nodemon" should work (without the path)
| const queryString = req.query.q; | ||
| const offset = Number((req.query.offset)?req.query.offset:'0'); | ||
| const limit = Number((req.query.limit)?req.query.limit:'10'); | ||
| const offset = Number((req.query.offset) ? req.query.offset : 0); |
There was a problem hiding this comment.
change to req.query.offset ?? 0 (it was added in node 14.0.0 https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Nullish_coalescing#browser_compatibility)
edit: and validate it's a positive number
| const offset = Number((req.query.offset)?req.query.offset:'0'); | ||
| const limit = Number((req.query.limit)?req.query.limit:'10'); | ||
| const offset = Number((req.query.offset) ? req.query.offset : 0); | ||
| const limit = Number((req.query.limit) ? req.query.limit : 10); |
There was a problem hiding this comment.
change to req.query.limit ?? 10
edit: and validate it's not over 500 (or any other number)
| res.json(result); | ||
| }); | ||
| }); | ||
| app.get('/api/1.0/movies/group_by/:field', async (req, res) => res.json(await searchService.getMovieGroupBy(req.params.field))); |
There was a problem hiding this comment.
https://www.npmjs.com/package/express-promise-router can use that to clean the code a bit
| app.listen(serverPort, () => { | ||
| console.log(`RediSearch Node listening at http://localhost:${serverPort}`); | ||
| }); | ||
| app.listen(serverPort, () => console.log(`RediSearch Node REST Server listening at http://localhost:${serverPort}`)); |
There was a problem hiding this comment.
the callback might get an error argument..
3 participants
This updates the Node Server to use Node 14+, top level await, modules, updates the Express dependency to the latest version and upgrades Node-Redis to version 4.