Skip to content

adding the model suffix #251

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

Closed
wants to merge 1 commit into from

Conversation

Traksewt
Copy link
Contributor

@Traksewt Traksewt commented Nov 3, 2016

Add a suffix to the core loopback models to allow multiple lb services to exist on the same client, pointing to APIs on different services.

The core models start with 'LoopBack'. The suffix is appended to the LoopBack part of the name while still appending its class after the suffix.

For example if the suffix is Accounts, then the generated core loopback models are: LoopBackAccountsResource, LoopBackAccountsAuth, LoopBackAccountsResourceProvider, LoopBackAccountsAuthRequestInterceptor.
See Issue #250

@slnode
Copy link

slnode commented Nov 3, 2016

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@slnode
Copy link

slnode commented Nov 3, 2016

Can one of the admins verify this patch?

1 similar comment
@slnode
Copy link

slnode commented Nov 3, 2016

Can one of the admins verify this patch?

@bajtos
Copy link
Member

bajtos commented Nov 9, 2016

Hello @Traksewt, thank you for the pull request!

I believe your needs should be already met by the recently added options "includeCommonModules", "namespaceModels" and "namespaceDelimiter" - see #219 for more details.

Could you please give it a try and let us know how far you could get?

If the current solution is not good enough, then I am proposing to stay consistent with the approach used in #219 and use ngModuleName as the namespace for the core client models (lbServices.LoopBackAuth,lbServices.LoopBackResource, etc.).

Also the proposed implementation is repeating name building too much. Could you please rework it to build the core model names (Auth, Resource, ResourceProvider, AuthRequestInterceptor) only once?

- LoopBack<%-: loopbackModelSuffix%>Auth
+<%-: loopbackAuth %>

@keithajackson what's your opinion on the change proposed in this pull request?
cc @davidcheung

@bajtos bajtos self-assigned this Nov 9, 2016
@bajtos
Copy link
Member

bajtos commented Nov 9, 2016

@slnode ok to test

@Traksewt
Copy link
Contributor Author

Traksewt commented Nov 9, 2016

Thanks for the feedback. A problem with excluding the common modules
completely is that then I won't be able to change the Base URL for the
different services. I was also trying to avoid namespacing the models, as
that would namespace all my modules (not just the common ones) which did
not make sense in my case.

I can look into reusing the ngModuleName, however, while dots in module
names are valid, they are very clunky in injectable service/factory names (
http://stackoverflow.com/questions/23638204/injecting-a-factory-with-dots-in-name).
I could try something like lbServicesLoopBackAuth ?

I can look at reworking the core model names in JS only once.

@bajtos
Copy link
Member

bajtos commented Nov 11, 2016

@Traksewt

problem with excluding the common modules
completely is that then I won't be able to change the Base URL for the
different services.

I see, this makes a lot of sense!

I was also trying to avoid namespacing the models, as
that would namespace all my modules (not just the common ones) which did
not make sense in my case.

This looks reasonable to me too.

What I would like to do is to come up with a nice consistent solution that would address all different needs related to using multiple LoopBack clients in the same application.

So far, I see the following "requirements":

  • Some users want to have one shared set of core services (LoopBackAuth et all), some users want each client to have its' own services.
  • Some users have the same model name in multiple clients and therefore need to prefix model names. Some users don't want to.

In that light, I am proposing to slightly change your proposal to the following:

  • Add a new option namespaceCommonModules that will enable namespacing of LoopBackAuth & co.
  • As for the namespacing algorithm, the following two options come to my mind (can you come up with more options?):
    1. Reuse ngModuleName and namespaceDelimiter for building the prefix. One can set { namespaceCommonModules: true, namespaceDelimiter: '' } to get lbServicesLoopBackAuth.
    2. Instead of adding (yet another) prefix, replace the existing LoopBack prefix with ngModuleName and ignore the namespaceDelimiter option. One can set { namespaceCommonModules: true } to get lbServicesAuth.

As I am re-reading my comment, the second option seems the best to me. I am imagining that when generating "Accounts" client, one will set { ngModuleName: 'Accounts', namespaceCommonModules: true } and get nice model names like AccountsAuth, AccountsResource, etc.

@keithajackson I would really like to get some input from you, as you are using multiple Angular clients too. What do you think?

@Traksewt
Copy link
Contributor Author

Traksewt commented Nov 14, 2016

@bajtos OK option 2 looks fairly good and neat. The one issue I have is that the style guides encourage to use dots for submodules in module names, but dots are not pretty for service names.
John Papa style guide for Angular1 modules

I could strip out the dot to work out the prefix, though that is a little obscure for the prefix generation (the user will need to know this so they can set the BaseURL). I'm talking about dots within the module name itself, I'm not talking about any delimiter between the new prefix (as you mentioned, there won't be any delimiter).

So I see:
a) leave it as it is (in option 2), dots in module names will break normal injection.
b) as option 2, but strip out the dots in the module name.
c) make namespaceCommonModules accept a string which becomes the new prefix, replacing Loopback.

@persimmons
Copy link
Contributor

persimmons commented Nov 14, 2016

Hey all!

Great discussion here - here's my late two cents:

If you're separately auth-ing in to two different LoopBack APIs, then this namespacing request totally makes sense. Minus hostname confusion (see below), the solution of allowing (optional) common module namespacing sounds good to me.

However: I see a hostname confusion edge case that could cause hard-to-trace failures if you are using multiple APIs hosted behind a proxy (which I'm guessing most of our sites in production will).

If you have a FE connecting to:

lbServicesAccounts @ urlBase: https://my.site.com/services/accounts
lbServicesThings @ urlBase: https://my.site.com/services/external/things

something weird happens at lines 373-377 from the generator:

          // filter out external requests
          var host = getHost(config.url);
          if (host && host !== urlBaseHost) {
            return config;
          }
          // ... set the request's access token

Both URLs will resolve to the same host per the getHost function - my.site.com - so both APIs will run the set-the-access-token code, resulting in unpredictably incorrect behavior based upon which lb-ng file gets loaded first.

Possible solutions I can think of: make urlBaseHost a predetermined value (which defaults to the current var urlBaseHost = getHost(urlBase) || location.host;), and change the http interceptor code slightly:

          // filter out external requests
          if (config.url && config.url.indexOf(urlBaseHost) === -1) {
            return config;
          }
          // ... set the request's access token

As far as I can tell, this should result in a non-breaking change that allows developers that want to auth against multiple API's to provide their own (more specific) base url strings, such as my.site.com/services/accounts or my.site.com/services/external/things.

As to the namespacing: please clarify if I missed something, but John Papa's comment on using dots for submodules doesn't apply here, since everything goes into the same module (ngModuleName). Thus, if I set:

{
  ngModuleName: 'lbServices',
  namespaceDelimiter: '',
  namespaceCommonModules: true
}

I should get a styleguide-compliant naming system:

module: 'lbServices'
  - service: 'lbServicesAuth'
  - model: 'lbServicesAccounts'

I really like @bajtos ' idea of just having one prefix, and the idea of auto-converting between dot and camel case worries me (auto-reformtting variable names/delimiters has been one of the most confusing "necessary evil" parts of Angular 1 itself in my experience).

@bajtos
Copy link
Member

bajtos commented Nov 22, 2016

Thank you @keithajackson for your thoughtful comment.

What's the best way to move this forward then? @Traksewt @keithajackson

@slnode
Copy link

slnode commented Nov 22, 2016

Can one of the admins verify this patch?

@Traksewt
Copy link
Contributor Author

OK, looking out for urlBaseHost sounds right.
We are already capturing urlBase, which seems what we want.

    this.setUrlBase = function(url) {
      urlBase = url;
      urlBaseHost = getHost(urlBase) || location.host;
    };

If urlBaseHost is going to contain path elements, could we just use urlBase instead? Or could urlBase be relative? If so, then will this give what we need?

urlBaseHost = getHost(urlBase) ? urlBase : location.host;

Regarding the dots, that is correct that everything goes into the one module, but the problem is if that module name actually does have a dot in it. E.g. if our generated module name is called 'Farm.Accounts', Then when you inject that and prepend it to a service, you will get a dot in the service name.

The easy answer is a), say you don't support it, but I (and possibly others in the future) will need to change our names which deviates from our standards. Not the end of the world, but just the generator won't be following best practices. We are using multiple loopbacks for many micro-services, so having some structure is helpful to us.

@bajtos
Copy link
Member

bajtos commented Dec 1, 2016

Regarding the dots, that is correct that everything goes into the one module, but the problem is if that module name actually does have a dot in it. E.g. if our generated module name is called 'Farm.Accounts', Then when you inject that and prepend it to a service, you will get a dot in the service name.

I see. Can we simply remove the dots from the generated service name? Or is that too hacky and/or difficult to grasp for SDK users?

{
  ngModuleName: 'Farm.Accounts',
  namespaceDelimiter: '',
  namespaceModels: true,
  namespaceCommonModules: true
}
// module name: Farm.Accounts
// a built-in service: FarmAccountsAuth
// a server model: FarmAccountsUser

I can also imagine that we can implement this part by replacing dots with the configured namespaceDelimiter (which is an empty string in the example above). It would allow users to configure different naming schemes, for example:

{
  ngModuleName: 'Farm.Accounts',
  namespaceDelimiter: '_',
  namespaceModels: true,
  namespaceCommonModules: true
}
// module name: Farm.Accounts
// a built-in service: Farm_Accounts_Auth
// a server model: Farm_Accounts_User

Thoughts?

To be honest, I don't have very strong opinion here and I am happy to accept any reasonable solution that supports all different use cases outlined in the previous discussion.

@bajtos
Copy link
Member

bajtos commented Feb 17, 2017

Hello, what's the status of this pull request?

@bajtos
Copy link
Member

bajtos commented Mar 2, 2017

I am closing this pull request as abandoned. Feel free to reopen if/when you get time to work on it again.

@Traksewt
Copy link
Contributor Author

Traksewt commented Apr 7, 2017

PR Finished in #269

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants