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

app.use('/location', service) vs app.service('/location', service) #287

Closed
farwayer opened this issue Apr 1, 2016 · 16 comments
Closed

app.use('/location', service) vs app.service('/location', service) #287

farwayer opened this issue Apr 1, 2016 · 16 comments

Comments

@farwayer
Copy link

farwayer commented Apr 1, 2016

For now service() makes two different things: getting and registering service. It makes more hard to understand core API. We already have use() function for registering service. In my opinion one function must make one thing and one thing must be done in one way to make API more consistence and code more clear. Maybe it will be better to move registering service in use() only in one of next major release?

@corymsmith
Copy link
Contributor

@farwayer use() and service() are two completely separate functions:

// Register the service
app.use('location', service);   
// Fetches the service to use somewhere else in your app
app.service('location')

@farwayer
Copy link
Author

farwayer commented Apr 1, 2016

You are not right. We can register service with app.service() also. This is described in docs and realized in code. We need to get out this functionality from service() function. It makes API inconsistent.

@corymsmith
Copy link
Contributor

@farwayer Can you send over the page in the docs where you see that?

@marshallswain
Copy link
Member

@farwayer, Use the service method for services. If you see other places where we're using use for a service, that should probably be changed in the documentation to avoid confusion, so please point it out. The use method is meant for initializing plugins.

That said, if you're only using app.service to deal with services, there's no confusion and the API is the same as that of any getter/setter.

app.service('/location', service) to set.
app.service('/location') to get.

@marshallswain
Copy link
Member

Actually, regarding plugins, I'm confusing app.use with app.configure, so never mind on that part.

@marshallswain
Copy link
Member

And... it's all over the docs. I agree that this is potentially confusing. Maybe I created that standard for myself, so I'll do some more checking.

@farwayer
Copy link
Author

farwayer commented Apr 1, 2016

Thank you for response but featcher generator generate code with use() as registering function. Also some internal services (authentication for ex.) use it. And one more thing: use() parse arguments for middlewares.

Maybe it is better to remove use() and use configure for plugins?

@corymsmith
Copy link
Contributor

The way its done in the generator is the recommended way.

Registering a service should be done with use() and service() should be used for fetching a service as its shown in the docs.

I'll let @ekryski or @daffl chime in if I'm incorrect :)

@farwayer
Copy link
Author

farwayer commented Apr 1, 2016

That is what I meant: no one know how is right 😄

@marshallswain
Copy link
Member

Well, it's interesting to me that it's all over the docs this way. :) I remember originally using app.service to register services and app.lookup to retrieve references to them, but then we did away with lookup and merged its functionality into service. use always worked to register services, but I guess as a personal preferences, I used service to differentiate between registering a service and a middleware. I can see in the code that use calls service internally, which is good, because we want to pass middleware to the providers. imo, all we need to do to clear up the potential for confusion is better explain the service method in the docs. I don't think we need to change all cases of use to into service, instead.

@farwayer
Copy link
Author

farwayer commented Apr 1, 2016

But if we need to add middleware must we use use()? Too many functions (configure, use and service) for basic things. It will confuse peoples. And it is use() everywhere in docs.

@marshallswain
Copy link
Member

Well, use is around as part of the Express roots. I don't think people will be confused if service is documented as both a getter and setter, but we stick to use in the docs.

@daffl
Copy link
Member

daffl commented Apr 1, 2016

app.use is an Express thing and will work for both to register a service if you pass a service object and for Express middleware if you pass a middleware function. I don't think we are using app.service anywhere in the documentation to register a new service except for mentioning that it is possible in the API docs for that method.

This is very likely to change for the next major version in #258 because not all frameworks have a use method but I think it should stay this way until then.

@farwayer
Copy link
Author

farwayer commented Apr 1, 2016

Thanks for discussion guys! It is very important to keep things simple. For now use() trying to 'understand' did caller pass service or express middleware. It can be error prone.

@daffl daffl added this to the 3.0.0 milestone Apr 1, 2016
@daffl
Copy link
Member

daffl commented Apr 1, 2016

That is true (see #285) which definitely needs a better error message. I think changing it to app.service consistently is a good suggestion but it shouldn't be done until the next major version (3.0) because otherwise it will make things even more confusing. We can take the documentation of app.service(name, service) out of the API docs for now if it is causing issues.

@daffl daffl removed this from the 3.0.0 milestone Apr 13, 2016
daffl pushed a commit that referenced this issue Aug 25, 2018
it will easily fix wrong generated table name.
daffl pushed a commit that referenced this issue Aug 29, 2018
* hashPassword fall-through if there's no password

The user should have already run a `requireAuth` hook by this point.

* JSHint
daffl pushed a commit that referenced this issue Aug 29, 2018
* hashPassword fall-through if there's no password

The user should have already run a `requireAuth` hook by this point.

* JSHint
@lock
Copy link

lock bot commented Feb 7, 2019

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue with a link to this issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Feb 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants