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

Allow localhost api address for backend #730

Merged
merged 5 commits into from
Jun 24, 2019
Merged

Conversation

ohmoses
Copy link
Contributor

@ohmoses ohmoses commented Jun 21, 2019

Closes #726

Allows to specify localHost and localPort in the Publish config—if present, the Publish backend will use this as the API address, while the frontend still uses host and port. E.g.:

"api": {
    "host": "https://api.example.com",
    "port": 443,
    "localHost": "http://localhost",
    "localPort": 8081
}

@ohmoses ohmoses requested a review from eduardoboucas June 21, 2019 08:29
Copy link
Contributor

@eduardoboucas eduardoboucas left a comment

Choose a reason for hiding this comment

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

Nice and simple! 👌

Can I just request a couple of changes in variable naming:

  • apiAddr -> apiAddress: we tend to avoid abbreviations when possible, as it may introduce ambiguity

  • localHost -> serverAccessHost, localPort -> serverAccessPort: I understand the reasoning behind using local here, as this feature will most likely be used for accessing an API that is local to the installation of Publish, but this may not necessarily be the case every time; a more encompassing terminology is "server access", as it describes the host/port used when the server accesses the API.

response.client = clients[0]
response.config = config.get()

const languagesPromise = request(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why create two separate variables with the result of the Promise and not just:

const {results: languages} = await request(
  `${apiAddress}/api/languages`,
  requestOptions
)

@ohmoses ohmoses force-pushed the feat/local-api-address branch from 580f14f to c1329d3 Compare June 24, 2019 13:46
@ohmoses ohmoses merged commit f888566 into develop Jun 24, 2019
@ohmoses ohmoses deleted the feat/local-api-address branch June 24, 2019 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Publish config to allow two locations for each API
2 participants