dustinfarris / ember-django-adapter

Ember CLI addon adapter for Django REST Framework
http://dustinfarris.github.io/ember-django-adapter
MIT License
191 stars 49 forks source link

Update to Ember-Data v3.0 #210

Closed OleRoel closed 5 years ago

OleRoel commented 6 years ago

My environment:

DEBUG: -------------------------------
ember-console.js:43 DEBUG: Ember                : 3.0.0
ember-console.js:43 DEBUG: Ember Data           : 2.18.2
ember-console.js:43 DEBUG: jQuery               : 3.3.1
ember-console.js:43 DEBUG: Ember Bootstrap      : 1.2.1
ember-console.js:43 DEBUG: Ember Django Adapter : 2.0.0
ember-console.js:43 DEBUG: Ember Simple Auth    : 1.5.1
ember-console.js:43 DEBUG: -------------------------------

The EDA 2.0 is still depending on "ember-data": "^2.17.0", which is finally resolved as ember-data-2.18.2.

I am currently developing an Ember FastBoot app and get connection errors for my REST API. This doesn't happen when the app is running as a regular Ember app. The FastBoot log doesn't give you a good hint where the problem lies, but in research for the cause of the issue I stumbled upon this: https://github.com/ember-fastboot/ember-cli-fastboot/issues/107. An update to a newer Ember-Data version should resolve Ember-Data and Fastboot issues.

dustinfarris commented 6 years ago

probably the right way forward here is to bump our dependency to ^3.0.0 and then do a major version release of ember-django-adapter as well

@OleRoel are you comfortable sending a pull request bumping the ember-data version?

OleRoel commented 6 years ago

Yes, I will do the pull request. In the meantime: yarn allows Ember-Data version to be forced to version 3 by adding

  "resolutions": {
    "ember-data": "3.0.2"
  }

to the package list: https://yarnpkg.com/lang/en/docs/selective-version-resolutions/

Sinled commented 6 years ago

Any updates on this?

OleRoel commented 6 years ago

Yes, I did a pull request which wasn't accepted by the CI:

https://github.com/dustinfarris/ember-django-adapter/pull/211

There was no response to my comment on this and I probably was a bit lazy to go deeper with my investigations, so the pull request didn't progress since March, 21st.

OleRoel commented 6 years ago

@dustinfarris can you take a look at my pull request https://github.com/dustinfarris/ember-django-adapter/pull/211? I'ld think that the you may have better insight than me if the one failing check that makes the acceptance test fail is really caused by the update to ember-data 3.0.0.

dustinfarris commented 6 years ago

@OleRoel I'm afraid I don't have time to dig deep right now. @Sinled do you have a few spare cycles to look at #211?

OleRoel commented 6 years ago

@dustinfarris @Sinled The acceptance test seems to fail because the test runner gets destroyed before the last test https://github.com/dustinfarris/ember-django-adapter/blob/b06a2127d8d9204bbb042002a0982af175aa108b/tests/acceptance/crud-failure-test.js#L219 has had the chance to finish up the save operation.

This is discussed here:

https://github.com/danielspaniel/ember-data-factory-guy/issues/104

and here:

https://github.com/emberjs/guides/issues/909

It looks like two extra lines of code might help to fix the issue:

test('Update field errors', function(assert) {
// ...some test code...
post.save().then({}, function(response) {
var done = assert.async();
// ...some test code...
done()
)}
// ...some more code

I would try to fix the async issue and open another pull request. But I am on vacation by Thursday and like to spend time with family. Which means that there is a chance that the pull request may hang for the next three weeks.

First, I will open up another issue...

danielspaniel commented 6 years ago

glad this issue was here. ember-data-factory-guy was barfing because of the multi ember-data resolution, so "resolutions": { "ember-data": "3.2" // or whatever version } was a great fix for time being, but this should still be fixed

archebyte commented 6 years ago

Same issue here, runing Django REST Adapter 2.0.0, will use yarn resolutions to see if it gets around this for now.

archebyte commented 5 years ago

Any progress on updating this dependency?

OleRoel commented 5 years ago

I think it depends on the fix for the failing acceptance test, which is an reported issue: https://github.com/dustinfarris/ember-django-adapter/issues/212

My bad, I haven't found time yet to fix this tiny problem-living in a start-up world sometimes put things on a higher priority in your todo list than fixing the obvious problems. I will try to do the fix for the acceptance test asap.

gojefferson commented 5 years ago

+1 on requests above to fix this. Being stuck on 2.x of Ember data not great.

benmurden commented 5 years ago

215 Will fix this.

Using newer ember-source with old ember-data is causing a lot of deprecation warnings, so it would be nice to see this merged soon.

dustinfarris commented 5 years ago

@benmurden CI is failing on master for ember-canary. Would you mind taking a look?

OleRoel commented 5 years ago

@benmurden @dustinfarris

The culprit lies in the canary build not returning a response string:

https://github.com/dustinfarris/ember-django-adapter/blob/master/tests/acceptance/crud-failure-test.js#L96

https://github.com/dustinfarris/ember-django-adapter/blob/master/tests/acceptance/crud-failure-test.js#L112

https://github.com/dustinfarris/ember-django-adapter/blob/master/tests/unit/adapters/drf-test.js#L91

This leads to:

not ok 7 Chrome 71.0 - [85 ms] - Acceptance: CRUD Failure: Permission denied error
    ---
        actual: >

        expected: >
            Unauthorized
        stack: >
                at http://localhost:7357/assets/tests.js:91:18
                at tryCatcher (http://localhost:7357/assets/vendor.js:70254:21)
                at invokeCallback (http://localhost:7357/assets/vendor.js:70427:33)
                at publish (http://localhost:7357/assets/vendor.js:70413:9)
                at publishRejection (http://localhost:7357/assets/vendor.js:70349:5)
                at http://localhost:7357/assets/vendor.js:64812:16
        negative: >
            false
        Log: |
    ...
not ok 8 Chrome 71.0 - [56 ms] - Acceptance: CRUD Failure: Server error
    ---
        actual: >

        expected: >
            Internal Server Error
        stack: >
                at http://localhost:7357/assets/tests.js:107:18
                at tryCatcher (http://localhost:7357/assets/vendor.js:70254:21)
                at invokeCallback (http://localhost:7357/assets/vendor.js:70427:33)
                at publish (http://localhost:7357/assets/vendor.js:70413:9)
                at publishRejection (http://localhost:7357/assets/vendor.js:70349:5)
                at http://localhost:7357/assets/vendor.js:64812:16
        negative: >
            false
        Log: |
    ...

and

not ok 58 Chrome 71.0 - [30 ms] - DRFAdapter: handleResponse - returns error with internal server error if 500
    ---
        actual: >

        expected: >
            Internal Server Error
        stack: >
                at Object.<anonymous> (http://localhost:7357/assets/tests.js:1147:14)
                at runTest (http://localhost:7357/assets/test-support.js:4394:30)
                at Test.run (http://localhost:7357/assets/test-support.js:4380:6)
                at http://localhost:7357/assets/test-support.js:4601:12
                at advanceTaskQueue (http://localhost:7357/assets/test-support.js:3993:6)
                at Object.advance (http://localhost:7357/assets/test-support.js:3974:4)
        negative: >
            false
        Log: |
    ...

Update

handleResponse should return something. No idea, why this is not happening in the canary build:

https://github.com/emberjs/data/blob/master/addon/adapters/rest.js#L901

Update 2 It looks like ember data canary build is also failing with the same issue:

https://travis-ci.org/emberjs/data/jobs/468500615#L634

A failing integration test is found here:

https://github.com/emberjs/data/blob/v3.6.0/tests/integration/adapter/rest-adapter-test.js#L2703

benmurden commented 5 years ago

@OleRoel Thanks for looking into this. These tests are not idempotent, so it'll be worth running them again when the issue is fixed upstream.

OleRoel commented 5 years ago

@benmurden @dustinfarris Should we ask upstream if someone is working on the issue or just wait for things to happen?

benmurden commented 5 years ago

Canary is likely to be flaky, so I wonder if it's a good idea to make its passing optional.

dustinfarris commented 5 years ago

I'm fine with that. Can you send a PR? -- Dustin Farris (646) 671-2007

On Jan 9, 2019, at 6:32 PM, Ben Murden notifications@github.com wrote:

Canary is likely to be flaky, so I wonder if it's a good idea to make its passing optional.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dustinfarris/ember-django-adapter/issues/210#issuecomment-452909661, or mute the thread https://github.com/notifications/unsubscribe-auth/ABCWvXXXNumPZExTK4S0LB_x31IYtbPCks5vBnwlgaJpZM4S7LKF.

benmurden commented 5 years ago

218 has this update

dustinfarris commented 5 years ago

Fixed in #215