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

Update to ember 1.13 serializer api. #114

Merged
merged 1 commit into from
Jul 24, 2015

Conversation

benkonrath
Copy link
Collaborator

I still need to update the documentation.

There is one remaining deprecation warning but it is an application level warning so I don't think we should override this function in EDA.

DEPRECATION: The default behavior of shouldReloadAll will change in Ember Data 2.0 to always return false when there is at least one "post" record in the store. If you would like to preserve the current behavior please override shouldReloadAll in your adapter:application and return true.

closes #110
closes #111

@benkonrath
Copy link
Collaborator Author

I'll work on updating the docs tomorrow morning.

@holandes22
Copy link
Collaborator

@benkonrath we currently do not test against the canary ember-data version in CI
You can enable that by changing the line in package.json:
"test": "ember try ember-data-stable" with `"test": "ember try:testall"

@holandes22
Copy link
Collaborator

@benkonrath oops, nvm my last comment, see you did that already :)

@benkonrath
Copy link
Collaborator Author

@holandes22 I have 2 test cases: ED 1.13.5 / E 1.12.1 and ED 1.13.5 / E 1.13.4. I didn't think it made sense to test against canary for our 1.0 release.

@holandes22
Copy link
Collaborator

I mentioned it since the changes you are making should make it work with the next version of ember-data (2.0). I think I made a mistake and called it canary and it is actually Beta.

Anyway, my idea was to also test against 2.0 as it should work with your modifications

@benkonrath
Copy link
Collaborator Author

@holandes22 The tests didn't pass with ember data and ember beta so I haven't included it in the test matrix.

@benkonrath
Copy link
Collaborator Author

Some notes on this update:

  • DRFSerializer now sub-classes JSONSerializer (as per ED suggestion for non-JSON API / legacy APIs)
  • Overriding metadata in extractMeta has changed
  • I removed the ember-cli-pagination instructions as that library doesn't seem to be maintained any more (it might still work though - I don't know off hand). I have wrote my own pagination compontent that I could contribute to this project if you guys think it fits.

@benkonrath benkonrath force-pushed the new-serializer-api branch 2 times, most recently from 1ea5b3c to 2aebc28 Compare July 16, 2015 09:55
@dustinfarris
Copy link
Owner

Embedded records seems to be broken.

@benkonrath
Copy link
Collaborator Author

Ok, I guess we should add a test for that. I'll check it out tomorrow.

@dustinfarris
Copy link
Owner

Looks like a bug in ED emberjs/data#3549

@benkonrath
Copy link
Collaborator Author

Thanks for finding that. I guess we have to wait for a new release before we can get this merged in.

@dustinfarris
Copy link
Owner

Yeah. Looks like a fix was merged yesterday: emberjs/data#3550

Just need a point release.

@benkonrath benkonrath force-pushed the new-serializer-api branch 4 times, most recently from e195645 to fa4bbf5 Compare July 20, 2015 13:44
@dustinfarris
Copy link
Owner

ED 1.13.6 is out and it looks like it has the bugfix we need

@benkonrath
Copy link
Collaborator Author

I can't grab it with bower yet. I'll update to ED 1.13.6 once I can.

@jpadilla
Copy link
Contributor

@benkonrath seems have been updated a couple of minutes ago.

https://github.com/components/ember-data

@benkonrath benkonrath force-pushed the new-serializer-api branch 2 times, most recently from 3bbce02 to 106942d Compare July 22, 2015 20:15
@benkonrath
Copy link
Collaborator Author

@jpadilla Thanks! I've updated the PR.

@benkonrath
Copy link
Collaborator Author

I'm having problems with the embedded records still. I'll investigate in a bit. I've updated the tests to expose the problem.

@benkonrath benkonrath force-pushed the new-serializer-api branch 2 times, most recently from 5a5a180 to a625340 Compare July 23, 2015 09:42
@benkonrath
Copy link
Collaborator Author

I made a mistake in the test. But the embedded record for belongsTo retrieve still isn't working in my app with this PR.

@benkonrath benkonrath force-pushed the new-serializer-api branch from a625340 to b6b1081 Compare July 23, 2015 09:46
@benkonrath
Copy link
Collaborator Author

I get this message in my app when trying to load a belongsTo embedded record:

Uncaught Error: Assertion Failed: You looked up the 'post' relationship on a 'comment' with id 2964 but some of the associated records were not loaded. Either make sure they are all loaded together with the parent record, or specify that the relationship is async (`DS.belongsTo({ async: true })`)

Everything is setup just like the test case. If I change to async: true, it tries to load the records with a separate request. @dustinfarris Can you try this version with your application to see if it's working with embedded records?

@benkonrath
Copy link
Collaborator Author

The embedded records are in records that are async loaded so that might be causing the problem. The ids that are being retrieved when I change to async: true are correct so ED resolves the embedded id but not the full record.

@benkonrath benkonrath force-pushed the new-serializer-api branch from d7c798c to e55c629 Compare July 23, 2015 21:09
@benkonrath
Copy link
Collaborator Author

User error! It's working ... I didn't update ember-data to 1.13.6 properly. :) It's ready for review.

@holandes22
Copy link
Collaborator

Awesome! I'll give it a try today

@dustinfarris
Copy link
Owner

Works beautifully on my end. Stellar work, as always!

@holandes22 please merge when ready.

@holandes22
Copy link
Collaborator

Works perfectly for me too. Amazing work, thank you!
Merging now

holandes22 added a commit that referenced this pull request Jul 24, 2015
Update to ember 1.13 serializer api.
@holandes22 holandes22 merged commit d6958eb into dustinfarris:master Jul 24, 2015
@jpadilla
Copy link
Contributor

Great stuff. Working for me too.

@Marthyn
Copy link

Marthyn commented Jul 31, 2015

Awesomeness! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants