Closed jollen closed 8 years ago
I have two comments with regards to this implementation.
1) Currently the property get syntax is the following: "/api/thing/property/get" which indicates to the server that we want to get a property of a thing, and then the router redirects the request to the handler which then returns the property of the things. The application uses the same syntax but different word in the third part of the URI, for patch (i.e. /api/thing/patch ) for actions (i.e. /api/thing/action) and for signalling events (i.e. /api/thing/eventsignall). The new implementation put the thing name before the action. I think normally the action should be the first so for example the syntax is GET /status/username to get the status of a user or GET /balance/username to get the account balance of a user. Also, why do we need to include the thing name in the URI? If it is a POST request then the thing name can be included in the POST body. As for the implementation details, I perhaps misunderstand, but I think the implementation is not quite correct. According the above description the response returns "{"thing":"switch12"}" which basically returns the thing name, but does not return the value of the property. It should return a particular property such as status of the switch (i.e.ON or OFF) or the power consumption or temperature.
2) It introduces an other dependency the morgan library for log handling. Currently the application uses the winston logging library. I understand Express can be wired up with morgan and most likely it does not support using winston, but such multiple log handlers could be a problem in deployment for example when we run the application as a service on Linux and we need to use third party runners such as PM2 or forever. I think we should use only one third party logging library, either winston or morgan. Probably it is not a good practice to use different log handler libraries for different tasks and areas.
Thanks for comments. Here are the replies.
1) We can put the thing name after action for sure. If the URI is in the syntax GET /username/status, the 'username' is considered as an instance (resource name), and 'status' is its property. It may more REST style. For the returns "{"thing":"switch12"}", I keep most of the simulator implementation. I haven't finished it.
2) Morgan is commonly used in combination with Winston. I use Morgan in order to log express transformation. The current Morgan "stream" will be replaced with Winston.
I have gone through on the modified and new files and now I understand the changes are related to the demo and simulator. If you prefer the devices that communicate with the WoT framework use Express instead of the restify library - which is currently used by the simulator - , then that's is perfectly fine and I think it's great we provider users with different implementation ideas.
I think it would be the best to leave the current "http_demo" and its device implementation as it is, so users who prefer using the less bloated restify library and want to stay away from Express in the device level would still have access to that example. To include your changes we could create a new directory for your Express based design, let say "http_express_demo" which could demonstrate the Express based implementation you propose. You could copy all modified and relevant files to that new directory. And then users who prefer Express for their underlying HTTP server would refer to the new "http_express_demo".
As for the logger, as it is a common file, perhaps you could add a configuration entry to your config file (which must be in your in the "http_express_demo" directory). The configuration entry could be like { "use_stream_log": true } and modify the logger.js file with an if clause to look up whether the "use_stream_log" does exists and it is true or not, and if yes then the logger will use the stream specific logger (morgan in your case).
Please let me know what you think.
It's great idea to leave the current "http_demo" and to create "http_express_demo". The "http_express_demo" demonstrate the Express based. And it also demostrates of how to use Express router to decouple the API implementation from the WoT server logic. The API router can then be reused by similar device WoT servers, said "http_express_demo2".
OK, so please go ahead and create the new directory and organize the files. You posted above that you haven't finish the design yet. Please let me know if it is completed and then I will merge the changes.
And as for the morgan logger. Since logger.js is a common file and it is at the framework level, I have moved morgan to demo application level to keep the original logger.js. It's better to start with a separate PR to handle the logger issue.
Yes, the issue "Use URL router to decouple the API" is completed with 464d512 and c002dfb. I would like to start the other PR to handle both of logger and API underlying implementation issues. And leave this PR at design level. This way contributors and you can review every single changes carefully.
Please also have discussions at #64. A simple CLI test case using curl:
Response:
The access log managed by morgan middleware: