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

Add deprecation for Route#render method #19442

Merged
merged 2 commits into from
Mar 2, 2021

Conversation

patocallaghan
Copy link
Contributor

@patocallaghan patocallaghan commented Mar 1, 2021

Adds deprecation for Route#render. See RFC 418 for more info. This deprecation has the same id/url as Route#renderTemplate as they were part of the same RFC.

id: 'route-render-template'
until: '4.0.0',
url: 'https://deprecations.emberjs.com/v3.x/#toc_route-render-template'
for: 'ember-source',
since: {
  enabled: '3.27.0',
},

Defaults to the return value of the Route's model hook
@private
*/
_render(name?: string, options?: PartialRenderOptions) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactors render to be _render, and call that from within renderTemplate and render instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use the symbol utility method from @ember/-internals/utils to make something more truly private.

@patocallaghan patocallaghan force-pushed the patoc/render-deprecation branch from d26e57b to 51d3614 Compare March 1, 2021 21:01
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation generally looks good, only had a couple small suggestions.

Defaults to the return value of the Route's model hook
@private
*/
_render(name?: string, options?: PartialRenderOptions) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use the symbol utility method from @ember/-internals/utils to make something more truly private.

let renderOptions = buildRenderOptions(this, name, options);
ROUTE_CONNECTIONS.get(this).push(renderOptions);
once(this._router, '_setOutlets');
deprecate('Usage of `render` is deprecated.', false, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets include some more information in the deprecation message. I'm thinking this.routeName at least...

@patocallaghan
Copy link
Contributor Author

@rwjblue I think I understand the symbol mechanism. Are you suggesting we do something like the following instead?

const RENDER = symbol('render');

// internal functionality now declared outside the class
function _render(name, options) {
  let renderOptions = buildRenderOptions(this, name, options);
  ROUTE_CONNECTIONS.get(this).push(renderOptions);
  once(this._router, '_setOutlets');
}

class Route extends EmberObject implements IRoute {
  constructor() {
    // ... snip ...
    // Make sure to bind to `this`
    this[RENDER] = _render.bind(this);
  }
  // ... snip ...
  renderTemplate(name, options) {
    this[RENDER]();
  }
}

@rwjblue
Copy link
Member

rwjblue commented Mar 1, 2021

Something more like:

const RENDER = symbol('render');

class Route extends EmberObject implements IRoute {
  [RENDER]() {
    // the implementation here
  }

  render() {
    // issue a deprecation
    return this[RENDER](...arguments);
  }
}

@patocallaghan patocallaghan force-pushed the patoc/render-deprecation branch from eda0c85 to dcf0819 Compare March 2, 2021 21:56
@patocallaghan patocallaghan force-pushed the patoc/render-deprecation branch from dcf0819 to 7765a67 Compare March 2, 2021 22:09
@patocallaghan
Copy link
Contributor Author

@rwjblue I've incorporated your feedback so I think it should be good to go now

@rwjblue rwjblue merged commit 70b73fd into emberjs:master Mar 2, 2021
@rwjblue
Copy link
Member

rwjblue commented Mar 2, 2021

Thank you!

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

Successfully merging this pull request may close these issues.

2 participants