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

ember-container-inject-owner - Checklist #12555

Closed
rwjblue opened this Issue Nov 4, 2015 · 19 comments

Comments

Projects
None yet
3 participants
@rwjblue
Member

rwjblue commented Nov 4, 2015

  • Implement FakeContainer concept discussed in #11874. This will help ensure full access to the container is possible, with a complete valid set of deprecations for each method or property accessed. The primary scenario that this affects that does not already have a deprecation is when the factory being instantiated does not have .extend.
  • Ensure ember-test-helpers detects getOwner / setOwner and functions properly with this feature on or off.
  • Document getOwner and setOwner functions in API docs. #12562
  • Audit and update guides details. I'm fairly sure we do not have any discussion of this.container in the guides (because it has been considered private API for ever), but we might want to add some prose to the dependency injection? emberjs/guides#938, needs another PR to explain getOwner usage.
  • Update ember-data to ensure deprecations are not triggered. emberjs/data#3912
  • Add deprecation guide entry for the this.container deprecation (added in emberjs/website#2409) (and cross link the deprecation URL (done in #12604)
  • Update liquid-fire to prevent deprecations. ember-animation/liquid-fire#388
  • Update ember-i18n jamesarosen/ember-i18n#336
@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Nov 4, 2015

Member

Looks complete to me. Thanks for putting together this list, @rwjblue.

Member

dgeb commented Nov 4, 2015

Looks complete to me. Thanks for putting together this list, @rwjblue.

@mixonic

This comment has been minimized.

Show comment
Hide comment
@mixonic

mixonic Nov 5, 2015

Member

@dgeb I will attempt docs for this stuff tonight and guides review. I hope that can free you up for some of the more tricky bits.

Member

mixonic commented Nov 5, 2015

@dgeb I will attempt docs for this stuff tonight and guides review. I hope that can free you up for some of the more tricky bits.

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Nov 5, 2015

Member

@mixonic thanks - you're the best! Ping me if you have questions about the new stuff.

Member

dgeb commented Nov 5, 2015

@mixonic thanks - you're the best! Ping me if you have questions about the new stuff.

@rwjblue rwjblue changed the title from ember-container-inject-owner - Checklist to Enable to ember-container-inject-owner - Checklist Nov 6, 2015

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 6, 2015

Member

Feature is enabled by default. I am working on ember-test-helpers updates to make sure this is handled properly.

Member

rwjblue commented Nov 6, 2015

Feature is enabled by default. I am working on ember-test-helpers updates to make sure this is handled properly.

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Nov 6, 2015

Member

Sounds good. I'll tackle the FakeContainer.

Member

dgeb commented Nov 6, 2015

Sounds good. I'll tackle the FakeContainer.

@mixonic

This comment has been minimized.

Show comment
Hide comment
@mixonic

mixonic Nov 8, 2015

Member

FWIW guides have been given a cursory review, though I have not undertaken any long prose about "dynamic lookup" which I think may be the only app-based use case for getOwner (other cases being around testing, and usually testing internals).

Member

mixonic commented Nov 8, 2015

FWIW guides have been given a cursory review, though I have not undertaken any long prose about "dynamic lookup" which I think may be the only app-based use case for getOwner (other cases being around testing, and usually testing internals).

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 9, 2015

Member
Member

rwjblue commented Nov 9, 2015

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 9, 2015

Member

I just published a polyfill that addons can use (https://github.com/rwjblue/ember-getowner-polyfill) pretty easily.

Member

rwjblue commented Nov 9, 2015

I just published a polyfill that addons can use (https://github.com/rwjblue/ember-getowner-polyfill) pretty easily.

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 9, 2015

Member

@tim-evans and I are working on updating ember-test-helpers to provide owner properly.

Member

rwjblue commented Nov 9, 2015

@tim-evans and I are working on updating ember-test-helpers to provide owner properly.

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 10, 2015

Member

ember-qunit and ember-mocha versions have been released that take advantage of the new feature (checking the ember-test-helpers item off on the checklist).

Member

rwjblue commented Nov 10, 2015

ember-qunit and ember-mocha versions have been released that take advantage of the new feature (checking the ember-test-helpers item off on the checklist).

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 10, 2015

Member

I took some time to update ember-data in emberjs/data#3912. I'm hoping that those changes can make it into Ember Data 2.2.0 (but Ember Data 2.3.0 would be perfectly fine).

Member

rwjblue commented Nov 10, 2015

I took some time to update ember-data in emberjs/data#3912. I'm hoping that those changes can make it into Ember Data 2.2.0 (but Ember Data 2.3.0 would be perfectly fine).

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 14, 2015

Member

ember-data PR is merged (checked off above), and liquid-fire PR is submitted (ember-animation/liquid-fire#388).

Member

rwjblue commented Nov 14, 2015

ember-data PR is merged (checked off above), and liquid-fire PR is submitted (ember-animation/liquid-fire#388).

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 14, 2015

Member

@dgeb - Have you begun work on the FakeContainer bullet point?

Member

rwjblue commented Nov 14, 2015

@dgeb - Have you begun work on the FakeContainer bullet point?

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Nov 14, 2015

Member

@rwjblue - sorry for the delay - I will try to wrap that up tomorrow

Member

dgeb commented Nov 14, 2015

@rwjblue - sorry for the delay - I will try to wrap that up tomorrow

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 14, 2015

Member

Added URL to the deprecation in #12604, and the deprecation guide in emberjs/website#2409.

Member

rwjblue commented Nov 14, 2015

Added URL to the deprecation in #12604, and the deprecation guide in emberjs/website#2409.

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 14, 2015

Member

@mixonic did the initial guide audit (mentioned in #12555 (comment)) and I just created emberjs/guides#982 to track updating the DI page with a few mentions of Ember.getOwner, so I am checking that item off for now.

Member

rwjblue commented Nov 14, 2015

@mixonic did the initial guide audit (mentioned in #12555 (comment)) and I just created emberjs/guides#982 to track updating the DI page with a few mentions of Ember.getOwner, so I am checking that item off for now.

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 14, 2015

Member

Updated ember-i18n in jamesarosen/ember-i18n#336.

Member

rwjblue commented Nov 14, 2015

Updated ember-i18n in jamesarosen/ember-i18n#336.

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Nov 14, 2015

Member

Implemented fake container injection in #12609

Member

dgeb commented Nov 14, 2015

Implemented fake container injection in #12609

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 16, 2015

Member

Thanks @dgeb, that fix is merged completing the last item on this checklist. Closing (please let me know if I have forgotten something).

Member

rwjblue commented Nov 16, 2015

Thanks @dgeb, that fix is merged completing the last item on this checklist. Closing (please let me know if I have forgotten something).

@rwjblue rwjblue closed this Nov 16, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment