State filter cnp integration #100
Conversation
Add Proxy to CNP methods Support Local Stores Query with distance
There was a problem hiding this comment.
It's great to see new work in this project. The code is generally in the idiom of the existing project and I see how this is coming together, and I'm excited to see it.
I did raise some concerns, which boil down to:
- Keep the coding style consistent - semicolons are expected, spaces are expected, objects should not be written all on one line, etc. Consider setting up
prettieroreslintso that there is no need for a lot of back and forth about it. - Please do not use
var, it causes confusion with variable scope, useconstorletas appropriate. - Server-side code should never trust inputs. If it is supposed to be a string, make sure it is a string. If you are building a query string for a URL, never build it by appending user input directly - since you are using Axios, use the built-in
paramsoption of Axios to build the query string easily by passing an object with the parameters and they will automatically get safely escaped.
Hope this is helpful! My apologies for the delay, I was busy getting married.
Happy to do some of these. If there are standards for the project why not commit .estlint config etc? |
There was a problem hiding this comment.
I see there's a consistent style being imposed somehow, which is great, I won't quibble about the change to double quotes. Made it a little hard to spot all the actual changes but no biggie.
My only real concern at this point is that sometimes parameters are taken from req.body or req.query without type checking and query strings are built in a way that wouldn't escape values correctly. Axios actually makes this easier:
axios.get('/api', {
params: {
id: 123,
name: 'John Doe'
}
})
That automatically builds the query string, escaping the values if they need it.
| const path = require('path'); | ||
| const manifest = require('../ssr/ssr-manifest.json'); | ||
| require("dotenv").config(); | ||
| const express = require("express"); |
There was a problem hiding this comment.
I guess I'm OK with the move from single quotes to double quotes as long as it's going to be consistent from here on out.
There was a problem hiding this comment.
That was just the default prettier config. I suggest if we want consistent rules let's get them in a .configuration file and add it to the project.
There are two new environment variables that need adding with the publish of this branch. I'll send those separately