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

Deprecate {{render helper #14441

Merged
merged 1 commit into from Nov 7, 2016

Conversation

Projects
None yet
8 participants
@josemarluedke
Contributor

josemarluedke commented Oct 11, 2016

This implements #13583.

Here is the TODO list from @mixonic.

  • Add the deprecation of {{render 'foo'}} in a PR.
  • Add tests for the deprecation of {{render 'foo'}}.
  • Update any tests that still use {{render 'foo'}} to test unrelated behavior to use modern idioms (components).
  • Audit the codebase for any remaining usage, for example in documentation, and refactor that to modern idioms.
  • Audit the guides for any remaining usage of {{render that can be removed.
  • Open a deprecation guide PR on the website.

I'm happy to work in the other items as well, but I'm not sure what to do with the current tests we have. Note that we have quite a few tests around render into from a route e.g. https://github.com/emberjs/ember.js/blob/master/packages/ember/tests/routing/basic_test.js#L1813
Also, there are several test failures because of the deprecation. Would love to get some guidance on how to update these tests and migrate the render into tests.

@ErikCH any change you still are interested in taking on some documentation?

@ErikCH

This comment has been minimized.

Show comment
Hide comment
@ErikCH

ErikCH Oct 11, 2016

@josemarluedke YES! Absolutely I can look at some documentation when we are ready and get that updated. Thanks for taking this on.

ErikCH commented Oct 11, 2016

@josemarluedke YES! Absolutely I can look at some documentation when we are ready and get that updated. Thanks for taking this on.

@mixonic

This comment has been minimized.

Show comment
Hide comment
@mixonic

mixonic Oct 11, 2016

Member

@josemarluedke I don't think you should attempt to deprecate this.render( on routes as part of this effort. Focusing on {{render in isolation would be the happy path.

Thanks for taking this on, please feel free to bug me for reviews etc as you move along :-D

Member

mixonic commented Oct 11, 2016

@josemarluedke I don't think you should attempt to deprecate this.render( on routes as part of this effort. Focusing on {{render in isolation would be the happy path.

Thanks for taking this on, please feel free to bug me for reviews etc as you move along :-D

@mixonic

This comment has been minimized.

Show comment
Hide comment
@mixonic

mixonic Oct 11, 2016

Member

Note that you can ignore deprecations in tests if there are tests for {{render you don't want to change or delete. There should be tests left over for {{render in the final version of this PR, but they should be limited to the {{render tests and not tests where the use of the deprecated helper is only incidental.

A deprecation guide and audit can totally happen in parallel with this effort, in fact that is even ideal as the two parts of the work can inform each other.

Member

mixonic commented Oct 11, 2016

Note that you can ignore deprecations in tests if there are tests for {{render you don't want to change or delete. There should be tests left over for {{render in the final version of this PR, but they should be limited to the {{render tests and not tests where the use of the deprecated helper is only incidental.

A deprecation guide and audit can totally happen in parallel with this effort, in fact that is even ideal as the two parts of the work can inform each other.

@josemarluedke josemarluedke changed the title from [WIP] Deprecate render to [WIP] Deprecate {{render Oct 11, 2016

@josemarluedke

This comment has been minimized.

Show comment
Hide comment
@josemarluedke

josemarluedke Oct 12, 2016

Contributor

Yes @mixonic, the goal is just to deprecate {{render not this.render. Updated the title to be more clear.

I didn't know about the ignoreDeprecation, that will definably help to make tests pass.

Should I leave the tests around render into then?

Contributor

josemarluedke commented Oct 12, 2016

Yes @mixonic, the goal is just to deprecate {{render not this.render. Updated the title to be more clear.

I didn't know about the ignoreDeprecation, that will definably help to make tests pass.

Should I leave the tests around render into then?

@josemarluedke josemarluedke changed the title from [WIP] Deprecate {{render to Deprecate {{render helper Oct 15, 2016

@josemarluedke

This comment has been minimized.

Show comment
Hide comment
@josemarluedke

josemarluedke Oct 15, 2016

Contributor

This is read for review.

I have added expectDeprecation in the tests that uses the render helper and now all the tests are passing.

I also checked any mentions in the guides for {{render and in the ember project itself, no usage of that.

The deprecation guide PR is here: emberjs/website#2700. It could be expanded further if necessary.

Contributor

josemarluedke commented Oct 15, 2016

This is read for review.

I have added expectDeprecation in the tests that uses the render helper and now all the tests are passing.

I also checked any mentions in the guides for {{render and in the ember project itself, no usage of that.

The deprecation guide PR is here: emberjs/website#2700. It could be expanded further if necessary.

@chadhietala

This comment has been minimized.

Show comment
Hide comment
@chadhietala

chadhietala Oct 15, 2016

Member

👍 I had started this in #14246, thanks for carrying it closer to the line. There is code in the router specifically for having a toplevel {{render}} that resolves to an {{outlet}}. To make sure we don't loose sight of that code (as we did in 2.0) when we eventually remove {{render}} can you please put a runtime deprecation here?

Member

chadhietala commented Oct 15, 2016

👍 I had started this in #14246, thanks for carrying it closer to the line. There is code in the router specifically for having a toplevel {{render}} that resolves to an {{outlet}}. To make sure we don't loose sight of that code (as we did in 2.0) when we eventually remove {{render}} can you please put a runtime deprecation here?

@josemarluedke

This comment has been minimized.

Show comment
Hide comment
@josemarluedke

josemarluedke Oct 15, 2016

Contributor

I have updated per @chadhietala request. Add a new runtime deprecation and updated the deprecation guide in the same PR as before (emberjs/website#2700).

This now closes #14246, #14097 and #13583.

Contributor

josemarluedke commented Oct 15, 2016

I have updated per @chadhietala request. Add a new runtime deprecation and updated the deprecation guide in the same PR as before (emberjs/website#2700).

This now closes #14246, #14097 and #13583.

@mmun

This comment has been minimized.

Show comment
Hide comment
@mmun

mmun Oct 15, 2016

Member

Nice work!

Member

mmun commented Oct 15, 2016

Nice work!

@mixonic mixonic merged commit 7147c32 into emberjs:master Nov 7, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mixonic

This comment has been minimized.

Show comment
Hide comment
@mixonic

mixonic Nov 7, 2016

Member

@josemarluedke @chadhietala @ErikCH Thanks very much for this work! ❤️

Member

mixonic commented Nov 7, 2016

@josemarluedke @chadhietala @ErikCH Thanks very much for this work! ❤️

@josemarluedke josemarluedke deleted the josemarluedke:deprecate-render branch Nov 7, 2016

@GavinJoyce

This comment has been minimized.

Show comment
Hide comment
@GavinJoyce

GavinJoyce Nov 8, 2016

Member

FYI this seems to have introduced some CI test failures:

https://travis-ci.org/emberjs/ember.js/jobs/173976039

update: the fix was simple, hardly worth the comment - #14588

Test failed: Helpers test: {{render}}:  uses `controller:basic` as the basis for a generated controller when none exists for specified name
    Failed assertion: Died on test #1   at generateTest (http://localhost:13141/ember.debug.js:43129:19)
    at forEach ([native code])
    at moduleFor (http://localhost:13141/ember.debug.js:43123:33)
    at http://localhost:13141/ember-tests.js:30697:44
    at internalRequire (http://localhost:13141/ember.debug.js:97:21)
    at requireModule (http://localhost:13141/ember.debug.js:44:29)
    at global code (http://localhost:13141/tests/?package=ember-glimmer:190:68): Please refactor `{{render "home"}}` to a component and invoke via `{{home}}`. ('-top-level' @ L1:C0) 
http://localhost:13141/ember.debug.js:5564
Test failed: Helpers test: {{render}}:  generates a controller if none exists
    Failed assertion: Died on test #1   at generateTest (http://localhost:13141/ember.debug.js:43129:19)
    at forEach ([native code])
    at moduleFor (http://localhost:13141/ember.debug.js:43123:33)
    at http://localhost:13141/ember-tests.js:30697:44
    at internalRequire (http://localhost:13141/ember.debug.js:97:21)
    at requireModule (http://localhost:13141/ember.debug.js:44:29)
    at global code (http://localhost:13141/tests/?package=ember-glimmer:190:68): Please refactor `{{render "home"}}` to a component and invoke via `{{home}}`. ('-top-level' @ L1:C11) 
http://localhost:13141/ember.debug.js:5564
Test failed: Helpers test: {{render}}:  should use controller with the same name as template if present
    Failed assertion: Died on test #1   at generateTest (http://localhost:13141/ember.debug.js:43129:19)
    at forEach ([native code])
    at moduleFor (http://localhost:13141/ember.debug.js:43123:33)
    at http://localhost:13141/ember-tests.js:30697:44
    at internalRequire (http://localhost:13141/ember.debug.js:97:21)
    at requireModule (http://localhost:13141/ember.debug.js:44:29)
    at global code (http://localhost:13141/tests/?package=ember-glimmer:190:68): Please refactor `{{render "home"}}` to a component and invoke via `{{home}}`. ('-top-level' @ L1:C11) 
http://localhost:13141/ember.debug.js:5564
Member

GavinJoyce commented Nov 8, 2016

FYI this seems to have introduced some CI test failures:

https://travis-ci.org/emberjs/ember.js/jobs/173976039

update: the fix was simple, hardly worth the comment - #14588

Test failed: Helpers test: {{render}}:  uses `controller:basic` as the basis for a generated controller when none exists for specified name
    Failed assertion: Died on test #1   at generateTest (http://localhost:13141/ember.debug.js:43129:19)
    at forEach ([native code])
    at moduleFor (http://localhost:13141/ember.debug.js:43123:33)
    at http://localhost:13141/ember-tests.js:30697:44
    at internalRequire (http://localhost:13141/ember.debug.js:97:21)
    at requireModule (http://localhost:13141/ember.debug.js:44:29)
    at global code (http://localhost:13141/tests/?package=ember-glimmer:190:68): Please refactor `{{render "home"}}` to a component and invoke via `{{home}}`. ('-top-level' @ L1:C0) 
http://localhost:13141/ember.debug.js:5564
Test failed: Helpers test: {{render}}:  generates a controller if none exists
    Failed assertion: Died on test #1   at generateTest (http://localhost:13141/ember.debug.js:43129:19)
    at forEach ([native code])
    at moduleFor (http://localhost:13141/ember.debug.js:43123:33)
    at http://localhost:13141/ember-tests.js:30697:44
    at internalRequire (http://localhost:13141/ember.debug.js:97:21)
    at requireModule (http://localhost:13141/ember.debug.js:44:29)
    at global code (http://localhost:13141/tests/?package=ember-glimmer:190:68): Please refactor `{{render "home"}}` to a component and invoke via `{{home}}`. ('-top-level' @ L1:C11) 
http://localhost:13141/ember.debug.js:5564
Test failed: Helpers test: {{render}}:  should use controller with the same name as template if present
    Failed assertion: Died on test #1   at generateTest (http://localhost:13141/ember.debug.js:43129:19)
    at forEach ([native code])
    at moduleFor (http://localhost:13141/ember.debug.js:43123:33)
    at http://localhost:13141/ember-tests.js:30697:44
    at internalRequire (http://localhost:13141/ember.debug.js:97:21)
    at requireModule (http://localhost:13141/ember.debug.js:44:29)
    at global code (http://localhost:13141/tests/?package=ember-glimmer:190:68): Please refactor `{{render "home"}}` to a component and invoke via `{{home}}`. ('-top-level' @ L1:C11) 
http://localhost:13141/ember.debug.js:5564
@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 8, 2016

Member

Thanks for fixing those in #14588. What happened here was that #14582 landed just a few minutes before this PR was merged (and it added these new render tests while fixing a bug). Those new tests didn't have the proper expectDeprecation (because the deprecation hadn't landed yet).

Member

rwjblue commented Nov 8, 2016

Thanks for fixing those in #14588. What happened here was that #14582 landed just a few minutes before this PR was merged (and it added these new render tests while fixing a bug). Those new tests didn't have the proper expectDeprecation (because the deprecation hadn't landed yet).

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