Added support for Node-Restify #353
Conversation
|
I am going to comment inline as I look through the code next. Hopefully you'll be able to git squash at the end to cleanup the actual pull request. |
| @@ -11,9 +11,9 @@ namespace NodeApi.Network { | |||
|
|
|||
| [ScriptImport] | |||
| [ScriptIgnoreNamespace] | |||
| public sealed class HttpServerRequest : ReadableStream, IEventEmitter { | |||
| public class HttpServerRequest : ReadableStream, IEventEmitter { | |||
There was a problem hiding this comment.
I didn't notice something in the restify docs to document the restify request deriving from the core http equivalents.
Independent of that though, I feel it might be better to simply duplicate, since:
- Not necessary everything on HttpServerRequest should be on the RestifyRequest.
- This opens up derivation possibilities in script# code which would neither be right, nor supported.
Same applies to HttpServerResponse.
Might be best duplicating the relevant properties.
There was a problem hiding this comment.
In Restify's Request API section, it says that it wraps all of the node ServerRequest API's, events and properties, plus the Restify specific entities.
I don't think we should be replicating functionality as we'll have to keep them in sync every time there are changes in Node's Request object. Same goes for Response.
I can duplicate for now, although I have not been successful at using git squash and have previous changes discarded after sending a pull request. Earlier I had to drop my repo and re-fork to get rid of some of the things. If you have any insights into how to do it, that'll be helpful.
There was a problem hiding this comment.
I don't think there's a way to undo changes/history for a specific file from an existing push.
I'm assuming that by using git commit --squash, you want me to bundle all the changes in a single request.
|
Thanks for all the detailed comments. I'll take a look at these and fix them when I get a chance. |
# The first commit's message is: Applied changes as suggested in the comments in #353 # This is the 2nd commit message: Added support for Node-Restify
|
I've squashed my changes. Hopefully I've done it right. |
| @@ -0,0 +1,20 @@ | |||
| | |||
There was a problem hiding this comment.
Weird, no idea where that showed up from. Removing.
|
|
||
| [ScriptIgnoreNamespace] | ||
| [ScriptImport] | ||
| public sealed class RestifyLogger { |
There was a problem hiding this comment.
What are the rules for construction here? Should this be creatable and have a public ctor? Or should it be internal? If the former, what is the ScriptName that should be specified on this?
There was a problem hiding this comment.
That's right, it should actually be private since its instance is returned through other properties and there's no way to create it for now. ScriptName should be object - seems like I'm missing a lot of those!
Changed.
|
Thanks again for your patience and the detailed CR. It is certainly helpful to have someone else look at this stuff especially when mapping to JS. Let me know if there are additional changes. I've started to think about tutorials and created my first how-to page on enlisting in the CC branch and using it locally. Some reason my gh-pages branch is deleted, so I can't send a pull request for it. Once this checkin goes through, I'll delete and refork the s# branch so I can send that doc. As I do more of these and get feedback, I'll get a good sense of how to structure tutorials as I move towards some more interesting ones. Next, I'm going to work towards a quick tutorial showcasing the AMD pattern and using SSLoader/RequireJS followed by a simple hello world type of example. It would be great if I can maintain continuity between tutorials. |
Removing files for initial node-restify commit
|
I'm doing something wrong in squash and hence you're seeing several checkins :|. Will need to look at a good tutorial for it. From what I understand I need to run git rebase -i and then pick the change sets and squash them into the latest one. However, I wonder if it got confused because I had already squashed an earlier one. |
|
I've seen various blogs/pages on using git rebase (see this one: https://help.github.com/articles/interactive-rebase) which suggest running: It shouldn't matter that you've already squashed earlier changes once. Effectively that resulted in a new item in the change history at that point, that you'd be squashing with a subsequent rebase command. If it works, then great. Otherwise, we'll just live with changes as they exist. If you try it again and it still doesn't help, just let me know and I'll merge as-is. Oh, and thanks for the patience as well, while incorporating the comments! :) |
Applied changes as suggested in the comments in #353 Applied changes as suggested in the comments in #353 Added support for Node-Restify Additional fixes to initial Node-restify library port Removing files for initial node-restify commit Removing files for initial node-restify commit Created node-restify stubs for s#
|
I seemed to have squashed it correctly, but ended up with a detached head. It asked me to sync with the remote and push changes. Now, some reason the commit shows as a merge. It does show 5 commits as part of it though, so maybe I got it right this time. |
|
Not sure what is happening - the intent was to see a single commit, while right now there are 7 commits in the pull request. |
Added support for Node-Restify
There were recent changes to their API in their documentation that I've accounted for the most part. If I've missed anything, I'll add it as I discover it. For now, I think this is a good starting point and we can continue to add missing stubs. Let me know if there are any fixes to this code that I should make. Thanks!