Skip to content

Conversation

@ef4
Copy link
Contributor

@ef4 ef4 commented Mar 15, 2018

The presence of the application adapter and serializer broke ember-animated's test suite (because I was already using ember-data for another purpose).

In general I think it's risky for an addon to customize the global default application adapter & serializer, even when the consuming app is "just" an addon dummy app.

This PR limits the scope of the adapter and serializer customizations to only the models that are part of ember-cli-addon-docs.

The presence of the application adapter and serializer broke ember-animated's test suite (because I was already using ember-data for another purpose).

In general I think it's risky for an addon to customize the global default application adapter & serializer, even when the consuming app is "just" an addon dummy app.

This PR limits the scope of the adapter and serializer customizations to only the models that are part of ember-cli-addon-docs.
@pzuraq
Copy link
Contributor

pzuraq commented Mar 15, 2018

I want to experiment with pulling the API documentation portion of the docs into an engine so we can get better isolation to prevent things like this from happening, but I think this is the best path forward for now!

@ef4
Copy link
Contributor Author

ef4 commented Mar 15, 2018

Yes, engines are a nice possibility for the future, but in their current state I don't think they'll meet your needs.

The biggest gap is that we'd want to allow the host application to inject some components into the engine (or "yield" from the engine back to a host app route). Otherwise you can't easily do fully interactive demos, etc.

@pzuraq
Copy link
Contributor

pzuraq commented Mar 15, 2018

My thinking is that the only portion of the app which would be an engine would be the API docs themselves, which contain all of the models/ember data needs. Basically everything in the /docs/api routes, which is not controlled by the user at all at this point

@pzuraq pzuraq merged commit 560bb02 into ember-learn:master Mar 15, 2018
@pzuraq
Copy link
Contributor

pzuraq commented Mar 15, 2018

Thanks for the PR 🎉

@dfreeman
Copy link
Contributor

Thanks, @ef4! This had been bugging me for a while but kept getting back-burnered for other things :)

@bravo-kernel
Copy link
Contributor

This is a huge fix @ef4, just spent about 6 hours investigating ember-cli-mirage only to end up here 🙄. Does this not warrant a patch-release ?

@pzuraq
Copy link
Contributor

pzuraq commented Mar 18, 2018

It does, there’ve just been quite a few things that went in/are going in so I wanted to wait until I had a chance to fully review tomorrow. Will see if I can get to it today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants