distilagency / starward

:boom: ReactJS Wordpress Boilerplate
27 stars 9 forks source link

Added blacklist regex for invalid characters #61

Closed falconmick closed 7 years ago

falconmick commented 7 years ago

bb_awards was having issues where Sketch was trying to copy unicode characters that broke client side redux. So far I cannot find a good whitelist solution that would support multi-lingual so I am instead using a blacklist. If a whitelist is found in the future that works multi-lingual this could be changed. For now this fixes my issues.

samlogan commented 7 years ago

@falconmick

How do you feel about enabling and disabling Redis from the config/app.js setup? That way we can have it disabled by default and the dev can easily switch it on or off in production or force enable in dev without needing to alter env variables on the server?

Adding something similar to the below at the bottom of this file:

// Redis
export const useRedis = false;
export const forceRedisInDev = false;
export const redisPrefix = '';

If useRedis or forceRedisInDev is set to true throw an error if redisPrefix is empty.

falconmick commented 7 years ago

That would be fine, I will start to implements this as time allows.

falconmick commented 7 years ago

@samlogan Just finished re-reviewing this as it's been a few days since we looked at it. I think that this PR is ready, It's now got redis + redis key prefixing aswell as a fix to stop auto-generated styles.css file from being uploaded + the orignal unicode blacklist filtering that the PR was made for!

The only thing left for discussion is if we should have a single redis client or multiple per request to create the client from our proxy. Currently if you were to need the redis proxy in another part of the node server you would start another client connection to redis + have the overhead of starting up the client. That being said, as we are using callbacks to handle request and initialising the client outside of the individual HTTP request we are NOT creating a client upon each request so having 2-10 clients shouldn't be a bad thing.

Opinion?

Cheers.

falconmick commented 7 years ago

Fixes #60

falconmick commented 7 years ago

Further reviewing has relieved: flushing not in these commits (being added now) and that we are not caching API requests, which means that after loading the website, redis provides 0 perf improvements for page loads once the user is already on the site. I am in the process of fixing flush, however I believe we can leave the non caching of the API endpoints until Apollo Client is implemented as initial page load is the primary reason for redis.

Feel free to leave any more feedback

falconmick commented 7 years ago

Redis Flush is now implemented. However the issue here: https://github.com/distilagency/starward_wp/issues/3 needs to be fixed as currently we need to do this manually for each site which sucks!