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

Added serializer blueprint and updated docs #124

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mattmarcum
Copy link
Contributor

This pr is a squashed version of #122

It adds a serializer blueprint and updates the docs.

This was referenced Mar 30, 2016
@nolanlawson
Copy link
Member

Sorry I haven't had a chance to look at this PR, could someone with more experience with Ember blueprints look at this please?

@fivetanley
Copy link
Contributor

@mattmarcum I think a serializers/application.js in the blueprints would be a great start. Users should get prompted if there is a conflict. But having it in the README could make it a first step that could lead to confusing bugs if missed.

@nolanlawson
Copy link
Member

@fivetanley So you mean this PR would make more sense without the README modification?

@fivetanley
Copy link
Contributor

We ran into this recently with the ActiveModelAdapter (this reminds me I need to fix it there too). Basically, if you don't define a serializer for a model, it will look for ApplicationSerializer, and then the default serializer for the adapter (in 2.0 this is JSONAPISerializer, 'RESTSerializer` in 1.x). The reason why the README modification was made (I'm guessing) is to account for this behavior. However, if we make having the PouchDB Adapter and Serializer be the default, this would do the right thing for users instead of asking them to correct this. This may be something we rework for Ember Data 3.0, but we can work around it in addons for now.

@fivetanley
Copy link
Contributor

fivetanley commented Apr 16, 2016

As somebody who usually skims for the installation step and then starts hacking, I think making the application.js for a default serializer/adapter is the move that sets users up for success.

So you mean this PR would make more sense without the README modification?

Yeah, if we add the files, that seems like a better solution to me.

@backspace
Copy link
Collaborator

I was reluctant to make it the default application serialiser/adapter but your point that it would prompt if there’s a conflict is true… so yeah, I also support adding it to the blueprint.

@simonexmachina
Copy link
Collaborator

@mattmarcum, thanks for this PR. I think the generator is the right approach - we actually hit this issue ourselves. It looks like we just need to update the README to direct people to use the generator and this can be merged.

@broerse
Copy link
Collaborator

broerse commented Jul 18, 2018

@aexmachina Can you make the readme change or close the PR?

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.

6 participants