Skip to content
This repository was archived by the owner on Jan 22, 2026. It is now read-only.

Api host proxy mixin#99

Merged
pixelhandler merged 5 commits intopixelhandler:masterfrom
realxsnl:api-host-proxy-mixin
Sep 10, 2016
Merged

Api host proxy mixin#99
pixelhandler merged 5 commits intopixelhandler:masterfrom
realxsnl:api-host-proxy-mixin

Conversation

@aars
Copy link
Collaborator

@aars aars commented Sep 6, 2016

This PR includes:

  • Better generated url from blueprint (uses API_HOST as well as API_PATH).
  • Sane default for adapter url through computed property (when not using generated url).
    • simplifies dummy app using this default.
  • Adds AdapterApiHostProxyMixin to keep example usage of API_HOST_PROXY in addon.
    • Removes confusing fetchUrl implementation from adapter blueprint
      • url = url.replace(proxy, host); !???? Was that supposed to be like that?

I've tried setting up the tests in an appropriate tests/unit/mixins file but I failed to get it running (missing owner, container, registry, etc). Test is added to adapter's unit test.

@pixelhandler
Copy link
Owner

@aars not sure if I mentioned it to you yet, I often use a test repo for manually testing too, see - https://github.com/pixelhandler/jr-test I think the dummy app works in this repo for testing, but needed an app that is actually consuming the addon (aside from dummy app behavior). If you have time would you consider some updates on the jr-test repo as well?

@class AdapterApiHostProxyMixin
@static
*/
export default Ember.Mixin.create({
Copy link
Owner

Choose a reason for hiding this comment

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

@aars thanks for adding this as a mixin :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My pleasure. When I find the time I'll look through all the docs. Those needs some touching up I reckon.

@pixelhandler
Copy link
Owner

pixelhandler commented Sep 6, 2016

@aars would you mind making one change to keep the config import in the blueprint?

import config from '../config/environment';

@aars
Copy link
Collaborator Author

aars commented Sep 7, 2016

@pixelhandler I messed up with the config import statement yeah.

When the blueprint fix/tests get merged we can add some more specific blueprint tests to catch this.

@aars
Copy link
Collaborator Author

aars commented Sep 7, 2016

@pixelhandler One "major" change in here is the reversal of API_HOST and API_HOST_PROXY. previously the proxy value got replaced by the host value... which... was very confusing to me at least, especially since API_HOST was set to '/' in all examples. Which isn't really a host.
(Ok I get it now.. '/' would mean "the same host the frontend runs on". I'll add a comment about this. I can probably improve the api-host-proxy-mixin example as well, something something trailing slashes.)

I thought it made more sense to define the API_HOST_PROXY as the actual proxy we're using to connect to API_HOST. So I did. But I can't really assess the impact of this change. I wonder how most users use this lib, and if they kept the original example and edited it to their needs, and thus, if this change will break their stuff. If we decide to keep this change it would be very worth updating docs and mentioning this in changelog before making a new release.

@pixelhandler pixelhandler merged commit 836121c into pixelhandler:master Sep 10, 2016
@aars aars deleted the api-host-proxy-mixin branch September 23, 2016 10:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants