Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for registering services dynamically #107

Merged
merged 5 commits into from
Feb 11, 2015

Conversation

marshallswain
Copy link
Member

This is Dynamic Services on top of the recent 1.0.2 patch. I've also made it no longer a requirement to pass a callback (#96). If the request fails, it logs a message to the console for a developer to track down.

As for consolidating the code a bit more. This is basically creating a second path in the code to setup a service. The first way is through the setup methods, and now this for sure works after the server has started. I'm not sure if it would be possible to just use this new way BEFORE the server starts. I would have to check to see if there is anything that the setup methods put in place that the services require as prerequisites.

Update: We for sure have to wait until the providers are set in place, which isn't really guaranteed until after the server starts (since providers are setup with configure). There may be other optimizations that can be made?

Closes #67 and closes #82

@marshallswain
Copy link
Member Author

For some reason I CAN create a branch on the main Feathers repo, but I'm not able to push to it, so there's a straggling branch called dynamic-services-2 hanging out right now. Update: Never mind, I wasn't able to push it, so there's no straggling branch after all.

@marshallswain marshallswain mentioned this pull request Feb 4, 2015
@daffl
Copy link
Member

daffl commented Feb 4, 2015

I just double checked and you should have push access to this repository. Did you use git@github.com:feathersjs/feathers.git?

@marshallswain
Copy link
Member Author

Ahh. I enabled two-factor auth. I forgot to use the personal access token. I pushed the branch.

@marshallswain
Copy link
Member Author

I think it looks good besides that but maybe we can discuss if there is a way to consolidate functionality even more.

@daffl What did you have in mind?

@@ -24,6 +24,19 @@ exports.setupMethodHandler = function setupMethodHandler (emitter, params, servi
args.splice(position, 0, {});
}
args[position] = _.extend({ query: args[position] }, params);

// If there was no callback supplied by a socket client...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we put this into a separate PR since it belongs to some different functionality?

@daffl
Copy link
Member

daffl commented Feb 5, 2015

Well it looks that for example the code from https://github.com/feathersjs/feathers/pull/107/files#diff-a89e33317f0c782705345838f29cf003R57 and https://github.com/feathersjs/feathers/pull/107/files#diff-dbb6eace96490fa4b721a67e6fd9340dR57 can be maybe consolidated into https://github.com/feathersjs/feathers/blob/master/lib/providers/socket/commons.js.

I'm also wondering if we could somehow hook into app.service and make sure that the provider methods can be called at any time? I think you a currently doing this in .use (at https://github.com/feathersjs/feathers/pull/107/files#diff-5372f626ee15242f1e2c6eb31655b4faR73) right?

@marshallswain
Copy link
Member Author

OK. I moved most of it into the commons. You'll notice that in the this.info part of each provider I put an attribute called connections, which is used in commons.js here. I put it in this.info because I didn't want to pollute the app's global space. hmm... Actually, the way it is set up, this.info gets mixed into the app, now. That makes it so that Socket.io can't be used at the same time as Primus. That's not really a concern, is it? It seems like you'd be using one or the other.

I left the setupMethodHandlers() method in each one since the methodHandlers depend on the connected socket and use different parameter names. It can be done, but it felt a little hacky moving it into commons.js.

@marshallswain
Copy link
Member Author

Oh. I also took that other commit regarding socket callbacks out.

@marshallswain
Copy link
Member Author

I'll make a PR for it after we get this one merged.

@daffl
Copy link
Member

daffl commented Feb 10, 2015

Sweet! I will check it out tonight and then merge it in. This will make for a great 1.1 release. Debating if ZeroRPC support would be something worth considering for that, too (and writing up how use this as microservices).

Oh and I'd like to polish the website a little, too.

daffl added a commit that referenced this pull request Feb 11, 2015
@daffl daffl merged commit d881599 into feathersjs:master Feb 11, 2015
@daffl daffl changed the title Dynamic services 2 Support for registering services dynamically Feb 11, 2015
@daffl
Copy link
Member

daffl commented Feb 11, 2015

Merged! Great job!

@daffl daffl added this to the 1.1.0 milestone Feb 11, 2015
@daffl daffl mentioned this pull request Feb 11, 2015
6 tasks
@marshallswain marshallswain deleted the dynamic-services-2 branch February 11, 2015 04:36
daffl added a commit that referenced this pull request Aug 21, 2018
daffl added a commit that referenced this pull request Aug 21, 2018
daffl pushed a commit that referenced this pull request Aug 24, 2018
…s-1.2.7

Update generator-feathers to the latest version 🚀
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamic services setup service method not called in async plugin
2 participants