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

Rename of Ember.Router.router to Ember.Router.routerMicrolib #14919

Merged
merged 1 commit into from
Feb 14, 2017

Conversation

scalvert
Copy link
Contributor

@scalvert scalvert commented Feb 9, 2017

The is an intermediate clean-up PR en route to finishing the router service RFC (see what I did there 👀 ).

Related PRs:
emberjs/website#2806
emberjs/website#2808

Reviewers
@rwjblue @locks @ef4

Changes

  • renamed Ember.router to Ember.routerMicrolib to disambiguate which router we're referring to. This will help avoid code like this.router.router...
  • added deprecation warning for the property
  • added tests for the deprecation warning

How to test drive

  • Checkout this branch
  • ember serve and ensure tests pass for both Enable Opt Features enabled and disabled

@@ -1563,4 +1564,9 @@ function representEmptyRoute(liveRoutes, defaultParentState, route) {
}
}

deprecateProperty(EmberRouter.prototype, 'router', 'routerMicrolib', {
id: 'ember-router.router',
until: '2.16'
Copy link
Member

Choose a reason for hiding this comment

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

Can you add the url property here (from the entry you are adding to the deprecations page on the website)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will definitely add once it's done!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

qq: how do i derive the URL from the section I added to the website? I see that there's a URL with a hash pointing to http://emberjs.com/deprecations/v2.x/#toc_*, but I'm not sure how it generates that URL.

I've been trying to run bundler for the website, but keep getting errors installing deps...

Copy link
Member

Choose a reason for hiding this comment

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

I merged the deprecation PR so that the site was auto-published:

http://emberjs.com/deprecations/v2.x/#toc_ember-router-router-renamed-to-ember-router-routermicrolib

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

let router = createRouter();

expectDeprecation(function() {
equal(router.router, router.routerMicrolib);
Copy link
Member

Choose a reason for hiding this comment

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

Can you tweak to assert.equal here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yessir!

@scalvert scalvert changed the title Rename of EmberRouter.router to EmberRouter.routerMicrolib Rename of Ember.Router.router to Ember.Router.routerMicrolib Feb 13, 2017
@rwjblue
Copy link
Member

rwjblue commented Feb 13, 2017

LGTM, can you squash commits down?

@krisselden
Copy link
Contributor

Should it be prefixed with an underscore?

@rwjblue
Copy link
Member

rwjblue commented Feb 13, 2017

@krisselden - Great point, and I can't believe I didn't think of that. Yes, I think we should underscore it so that it is router._routerMicrolib...

@scalvert - Thoughts?

@scalvert
Copy link
Contributor Author

Yes to squashing. Yes to prefix. Good call @krisselden!

@scalvert
Copy link
Contributor Author

@rwjblue @krisselden renamed routerMicrolib to _routerMicrolib and squashed commits. Also updated the associated deprecations docs on the website (see second related PR in the description above)

@chadhietala chadhietala merged commit a821583 into emberjs:master Feb 14, 2017
@rwjblue
Copy link
Member

rwjblue commented Feb 14, 2017

Awesome, thank you @scalvert!

@scalvert scalvert deleted the rename-router branch March 3, 2017 03:42
@stefanpenner
Copy link
Member

This PR should likely have created a accessor that emits a deprecation for the Router.router property.

Some (debug related) workflows relied on Router.router to get a list of routes (for example: https://github.com/stefanpenner/ember-console/blob/master/lib/commands/console.js#L69)

Also #15235

Although this was private, ensuring we have a helpful error when accessing the property is most likely appropriate

@rwjblue
Copy link
Member

rwjblue commented Jun 1, 2017

@stefanpenner - Please feel free to review the diff of this PR, it actually adds tests for exactly that scenario.

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

Successfully merging this pull request may close these issues.

5 participants