search

knownasilya / ember-plupload

Ember component for handling uploads using plupload
MIT License
87 stars 53 forks source link

Update everything #134

Closed knownasilya closed 4 years ago

knownasilya commented 5 years ago

WIP, don't merge.

corrspt commented 5 years ago

Hi @knownasilya :)

I'm currently using this addon (works great, thanks for maintaining it). I Just tried updating to Ember 3.8 and apparently Ember 3.8 removed the "Ember.Map" that's used in the init function of the uploader service

init() {
    set(this, 'queues', Ember.Map.create()); // This line
    set(this, 'all', Ember.A());
  },

Making this addon incompatible with Ember 3.8 I guess (It works in 3.7). I've seen that this pull request is fixing (also) exactly this situation, but the Pull Request is open for a while. Is there something I can do to help you out?

Best regards

knownasilya commented 5 years ago

@corrspt would love if you could tackle any of the todo items above that are open. Feel free to just submit a PR against this PR.

corrspt commented 5 years ago

@knownasilya Got it, I'll do my best! Thanks for the quick reply :)

corrspt commented 5 years ago

Hey @knownasilya I started working on this yesterday and one the items here is to convert unit test in integration tests.

I ran into the issue that unit tests have access to internal stuff of Ember Plupload, not really sure how to tackle that (example of getting the upload queue: https://github.com/knownasilya/ember-plupload/blob/master/tests/unit/components/pl-uploader-test.js#L166-L176).

Should some of those tests be deleted? Maintained as unit tests? How would I go about converting them? I've seen in the PR an example of converting a test (which was really useful). Maybe I just need to get more acquainted on how things work, but would appreciate a little help :)

Thanks :)

knownasilya commented 5 years ago

I think we can do both. In some places it makes sense to move a unit test to integration because it happens to use internal state but can actually be tested in another way. I'm not against keeping some of the tests as unit where it makes sense. Also we could probably do followup prs later.

corrspt commented 5 years ago

Hi @knownasilya

Very sorry for the long delay in replying. I couldn't migrate the tests after some work and I missed my "upgrade time" and I haven't done anything in the last few months regarding this. I hope to get another upgrade time soon and take a stab at this.

I did spend sometime looking at the unit tests, but several of them I really didn't know how to migrate as they seemed to test some internal stuff (on the other hand they were failing, and I also didn't know how to make them pass).

knownasilya commented 5 years ago

@corrspt no problem. I might have a look this weekend.