Default to relative path instead of localhost for domain.

[warmans]

Changes the template to use the window location as the default domain. This seems like a more sensible default. In general it seems to be rather awkward to inject the domain when switching between different API environments (e.g. staging vs prod deployment).

I've not regenerated the code or anything but I assume I didn't break anything.

[vmasek]

@warmans Template isn't the right place to put the other default behavior like this.

Method generateDomain is responsible for the default localhost because there was no host defined in the swagger file.

You can easily achieve your desired behavior with window.location as a base if you inject the domain in the module where you are providing the client service.

@NgModule({
  declarations: [
    AppComponent
  ],
  imports: [
    HttpClientModule,
    /* ... */
  ],
  providers: [
    ApiClientService,
    {
      provide: 'domain',
      useValue: `${window.location.hostname}${window.location.port ? ":" + window.location.port : ""}`
      // or use the value that is defined in your current environment as environment.apiUrl
    },
    /* ... */
  ],
  bootstrap: [AppComponent]
})
export class AppModule {
}

[warmans]

@vmasek I was actually doing that but it seems to break when using an AOT build. ng serve works fine but the aot build gives me null/api/v1/..... I'm not sure why. Either way I figured if a domain cannot be identified it makes more sense to use a relative path than to just default to some random host/port.

[vmasek]

@warmans I agree, best solution will be to move the logic into the generator method generateDomain

It will generate host from the swagger file (if provided) or assign the template string that will be placed in mustache template as an initializer for the domain.

[warmans]

hmm, how do you use the template string in that context? Something like this?

  private static generateDomain({schemes, host, basePath}: Swagger): string {
    const protocol =
      schemes && schemes.length > 0
        ? `${schemes[0]}://`
        : '//';
    const domain = host ? host : "`${window.location.hostname}${window.location.port ? ':'+window.location.port : ''}`";
    const base = ('/' === basePath ? '' : basePath);
    return `${protocol}${domain}${base}`;
  }

[vmasek]

I would do it like this

private static generateDomain({schemes, host, basePath}: Swagger): string {
    const protocol =
      schemes && schemes.length > 0
        ? schemes[0]
        : 'http';
    const domain = host ? host : "${window.location.hostname}${window.location.port ? ':' + window.location.port : ''}";
    const base = ('/' === basePath ? '' : basePath);

    return `${protocol}://${domain}${base}`;
  }

also do not forget to add interpolation with no escape in templates/ngx-service.mustache

  private domain = `{{&domain}}`;

[vmasek]

LGTM

@warmans we have added the CLA that is required to be signed before PR can be merged

[CLAassistant]

All committers have signed the CLA.

[warmans]

I've had to make a couple of changes:

I think the scheme stuff is a bit dodgy since it appears to just select the first scheme defined in the swagger. In cases where it infers the URL I think it should just the "whatever" http scheme of // so I've added a check that host is defined before using the schemes scheme.

   host && schemes && schemes.length > 0
        ? schemes[0]         +        ? `${schemes[0]}://`
        : 'http';        +        : '//';

For the basepath apparently I do not have this defined in my swagger file so I get a "null" string in the URL. I've added a check that it appears to be something truthy.

I need to do a bit more testing but it's Friday evening so I'll probably do it next week.

[warmans]

I think this is good to go unless there are final comments.

[vmasek]

Will look on it, I am now OOO for few days. Also, @gelidus please review the changes.

@warmans The CLAassistant needs the CLA to be signed before we can merge the PR

[warmans]

@vmasek the CLA thing is green for me. Maybe you just need to refresh it.