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

[BUGFIX beta] deprecate this.resource() in Router.map() #11517

Merged
merged 2 commits into from
Jun 23, 2015

Conversation

jayphelps
Copy link
Contributor

Use this.route('name', { resetNamespace: true }) instead.

This has been talked about for months, but hadn't seen a PR so figured 2.0 beta it was time.

Back in March, this.resource was removed from the guides and released as 1.11.0.

@jayphelps
Copy link
Contributor Author

I assume the prod build failed because of deprecation stripping in env=prod. Not sure how to counter this? (without removing the expect(count)) Switched to the more general, top-level expectDeprecation usage. Seemed to fix it locally.

Test failed: Ember Router DSL: should fail when using a reserved route name
    Failed assertion: Expected 12 assertions, but 8 were run

https://travis-ci.org/emberjs/ember.js/jobs/67565607
https://github.com/emberjs/ember.js/pull/11517/files#diff-67ac84b7cb119834ff0e6318cf17cb03L18

…te('name', { resetNamespace: true }) instead
@jayphelps jayphelps force-pushed the deprecate-router-map-resource branch from b873fa0 to 78c22ee Compare June 19, 2015 19:13
@jayphelps
Copy link
Contributor Author

All green.

@workmanw
Copy link

I'm just curious, what's the reason for this? If this.resource is the same as this.route('name', { resetNamespace: true }), why bother?

@jayphelps
Copy link
Contributor Author

@workmanw core team has their own rationale I'm sure, but IMO it's always been a huge confusion point for new-comers. Like, almost unanimously everyone is confused when I see them learning ember. Since you can nest this.route now, having the explicit resetNamespace: true makes it very more clear what you intended vs. "wtf why isn't ember finding my route/model/template/etc".

The original intent was to have a concept of a resource mapping effectively for things that are models (hence the name) but it just made things more confusing and inconsistently used and abused.

@workmanw
Copy link

Gotcha. I hadn't really heard anything about this change and I was curious if there was some other plan for this.route or this.resource.

I often see changes like this and am always curious about the rationale. Not questioning it. Just trying to better understand the motivation for the change.

@jayphelps
Copy link
Contributor Author

@workmanw this particular case was definitely more informally discussed on IRC/slack/discuss.emberjs.com/etc. It's possible the core team has collectively not agreed to move forward on this, I just happened to be refactoring code and had the cycles to PR.

@rwjblue
Copy link
Member

rwjblue commented Jun 19, 2015

I am in favor of this PR, but there are a few things that will still be needed:

  • PR updating guides
  • Adding API documentation for resetNamespace

I'll confirm with others, but I believe that this is exactly the direction we have been intending to go...

@jayphelps
Copy link
Contributor Author

@rwjblue happy to do both once someone confirms this/they will be merged. Thanks!

@jmurphyau
Copy link
Contributor

I'm wondering if somehow, while the Router.map is being discussed, I can sneak in and ask for an opinion on #11308 ...

Might be something for another time - needing more discussion etc - but if not then might be a good time to get two changes in at once or combine it into one PR or something?

Given that resource is to be removed it might mean the idea behind my PR should be implemented differently

@jayphelps
Copy link
Contributor Author

@jmurphyau IMO definitely a different discussion, although I do like the idea of your PR at first glance!

@jmurphyau
Copy link
Contributor

👍 sweet - sounds good.. Had no activity on it since I opened it and seen this PR as a chance to steal some attention ;)

BTW +1 to route/resource being a pain point.. It was for me.. A resetNamespace option is a much better implementation.

jayphelps added a commit to jayphelps/guides that referenced this pull request Jun 22, 2015
@jayphelps
Copy link
Contributor Author

Can someone kick saucelabs for me? I believe this is should be green and all requests by @rwjblue for docs should be ready, but IE test timed out
image

@stefanpenner
Copy link
Member

kicked

rwjblue added a commit that referenced this pull request Jun 23, 2015
[BUGFIX beta] deprecate this.resource() in Router.map()
@rwjblue rwjblue merged commit 49d5853 into emberjs:master Jun 23, 2015
@rwjblue
Copy link
Member

rwjblue commented Jun 23, 2015

Thanks @jayphelps!

@tim-evans
Copy link
Contributor

As a follow up, could we have an ember-watson task for this?

@rwjblue
Copy link
Member

rwjblue commented Jun 24, 2015

@tim-evans - Seems good to me, would you mind making an issue over in ember-watson repo explaining what needs to be done?

clekstro added a commit to fauxton/travis-web that referenced this pull request Jan 31, 2016
The use of `resource` within the router is deprecated as of 2.0.0.
See notice [here](emberjs/ember.js#11517).

This uses ember-watson to do the manual conversion, with small
aesthetic tweaks.
clekstro added a commit to fauxton/travis-web that referenced this pull request Feb 1, 2016
The use of `resource` within the router is deprecated as of 2.0.0.
See notice [here](emberjs/ember.js#11517).

This uses ember-watson to do the manual conversion, with small
aesthetic tweaks.
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.

6 participants