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

Multiple bug fixes. #124

Merged
merged 2 commits into from
Aug 27, 2015
Merged

Multiple bug fixes. #124

merged 2 commits into from
Aug 27, 2015

Conversation

Kilowhisky
Copy link
Contributor

Fixes issues
#123
#68

Chris Rice added 2 commits July 2, 2015 08:34
…e instead of direct comparison. This fixes issue where each model has their own declared model adapter like ember-cli now requires. References issue: locks#68
@Kilowhisky Kilowhisky changed the title Fix bug where findQuery no longer works in 0.5.4 Multiple bug fixes. Jul 2, 2015
@kurko
Copy link
Collaborator

kurko commented Jul 2, 2015

Can you fix these failures as well, @Kilowhisky?

@Kilowhisky
Copy link
Contributor Author

@kurko the build failures you're seeing is because of this bug. #119 Nothing to do with the changes i've made.

I can give it a shot but i'm not very well versed in the test-suite.

@Kilowhisky
Copy link
Contributor Author

Actually the errors are because of breaking changes in ember data >= 1.13.0. If you run the test runner based on ember data 1.0.0-beta.19.2 there are no errors.

In order to fix them you either need to specify the ember data dependency at '1.0.0-beta.19.2' or declare possible version requirement going forward.

@Kilowhisky
Copy link
Contributor Author

Also is there a reason why the recursive model loading only travels 1 level down?

@kurko
Copy link
Collaborator

kurko commented Jul 16, 2015

I'm a little wary of merging this without certainty that the tests are actually passing for this set of features... I'm not using the lib right now in any project, so it's hard for me to work on it.

@Kilowhisky
Copy link
Contributor Author

So i have a branch that almost passes the tests but is failing on one last bug that i think is in ember data but others don't seem believe so or are staying silent. Can you take a look at this test and give me your thoughts on how to proceed?

https://github.com/Kilowhisky/ember-localstorage-adapter/tree/ember-1-13

Here is my open bug report on ember-data btw.

emberjs/data#3614

@kurko
Copy link
Collaborator

kurko commented Aug 7, 2015

For some reason, it's extremely hard to follow the master...Kilowhisky:ember-1-13 diff. It looks like everything is being replaced.

I think that as for the ED bug, you'd need to open a PR there with a failing test in ED (not local storage). I've been on this road for almost 3 years and I now they will not give too much attention unless you're talking about ED. Anything on other adapters seem to simply be "the problem is the adapter".

@kurko
Copy link
Collaborator

kurko commented Aug 7, 2015

Regarding

{
  'list': {
    records: {
      'l1': { id: 'l1', name: 'one', b: true, items: ['i1', 'i2'] },
      'l2': { id: 'l2', name: 'two', b: false, items: [] },
      'l3': { id: 'l3', name: 'three', b: false, items: [] }
    }
  }
}

I think we need to filter this because you really shouldn't return items there (list l1). If you set async: true, then it's fine because a second call will be made to get items.

But if you set async: false, then you should be returning the included data altogether in the same response.

@Kilowhisky
Copy link
Contributor Author

The diff is because of a line ending screw up. I'll try and see if i can revert it.

@Kilowhisky
Copy link
Contributor Author

Here is a branch with a much cleaner diff.
https://github.com/Kilowhisky/ember-localstorage-adapter/tree/ember-1.13.+

@kurko
Copy link
Collaborator

kurko commented Aug 18, 2015

Right. Can you reopen against the new branch? 😄

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.

None yet

2 participants