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.

Sign up for GitHub

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

Jump to bottom

RFC: Deprecate And Remove Spree.routes #3405

Closed
jarednorman opened this issue Oct 25, 2019 · 2 comments
Closed

RFC: Deprecate And Remove Spree.routes #3405

jarednorman opened this issue Oct 25, 2019 · 2 comments

Comments

@jarednorman
Copy link
Member

As documented in #2113, the route helpers in Spree.routes are poorly and inconsistently named. Hawth also stopped using them in new admin code, instead preferring to use pathFor directly. As mentioned in #2113, this is a pretty low priority issue, but making a decision allows us to close the old issue and move on with a solution.

Solution:
Deprecate Spree.routes and replace all uses of it with pathFor. Remove it in a future major version. I prefer this solution because Spree.routes doesn't provide much value and is a small burden to keep up to date.

Alternative:
Warn users that we will be making Spree.routes consistent with the Rails route naming. Update all the names in the next major version. Given that this is still a breaking change, I'm less a fan of it, but if people truly find Spree.routes valuable, then I'm not opposed to keeping it.
@jarednorman
Copy link
Member Author

jarednorman commented Oct 25, 2019 •
edited
Loading

Now that we have this issue, someone with permissions can close #2113.

seand7565 added a commit to seand7565/solidus that referenced this issue May 4, 2020
@seand7565
Replace Spree.routes with Spree.pathFor
3c28685 
This PR replaces all instances of Spree.routes with Spree.pathFor. Spree.routes still
exists, we're just preferring to use Spree.pathFor per solidusio#3405.
@seand7565 seand7565 mentioned this issue May 4, 2020
Merged
5 tasks
seand7565 added a commit to seand7565/solidus that referenced this issue May 12, 2020
@seand7565
Replace Spree.routes with Spree.pathFor
b418c69 
This PR replaces all instances of Spree.routes with Spree.pathFor. Spree.routes still
exists, we're just preferring to use Spree.pathFor per solidusio#3405.
seand7565 added a commit to seand7565/solidus that referenced this issue May 12, 2020
@seand7565
Replace Spree.routes with Spree.pathFor
79a9a54 
This PR replaces all instances of Spree.routes with Spree.pathFor. Spree.routes still
exists, we're just preferring to use Spree.pathFor per solidusio#3405.
seand7565 added a commit to seand7565/solidus that referenced this issue May 12, 2020
@seand7565
Replace Spree.routes with Spree.pathFor
253bfdb 
This PR replaces all instances of Spree.routes with Spree.pathFor. Spree.routes still
exists, we're just preferring to use Spree.pathFor per solidusio#3405.
seand7565 added a commit to seand7565/solidus that referenced this issue May 13, 2020
@seand7565
Replace Spree.routes with Spree.pathFor
a2e1ad9 
This PR replaces all instances of Spree.routes with Spree.pathFor. Spree.routes still
exists, we're just preferring to use Spree.pathFor per solidusio#3405.
seand7565 added a commit to seand7565/solidus that referenced this issue May 16, 2020
@seand7565
Replace Spree.routes with Spree.pathFor
63258f3 
This PR replaces all instances of Spree.routes with Spree.pathFor. Spree.routes still
exists, we're just preferring to use Spree.pathFor per solidusio#3405.

It uses Proxy to issue a deprecation warning, however if Proxy isn't defined (in older
browsers like IE) will just return the routes without deprecation. Hopefully the developer
of the app will have something newer than IE, if they don't I imagine they have
bigger problems than JS route deprecations 😬

Also adds Proxy to ESlint so it doesn't trigger the no-undef rule.
seand7565 added a commit to seand7565/solidus that referenced this issue May 22, 2020
@seand7565
Replace Spree.routes with Spree.pathFor
5d55e10 
This PR replaces all instances of Spree.routes with Spree.pathFor. Spree.routes still
exists, we're just preferring to use Spree.pathFor per solidusio#3405.

It uses Proxy to issue a deprecation warning, however if Proxy isn't defined (in older
browsers like IE) will just return the routes without deprecation. Hopefully the developer
of the app will have something newer than IE, if they don't I imagine they have
bigger problems than JS route deprecations 😬

Also adds Proxy to ESlint so it doesn't trigger the no-undef rule.
@seand7565
Copy link
Contributor

#3605 is merged and should close this issue! 🎉

mamhoff pushed a commit to mamhoff/solidus that referenced this issue Feb 1, 2021
@seand7565 @mamhoff
Replace Spree.routes with Spree.pathFor
0423f62 
This PR replaces all instances of Spree.routes with Spree.pathFor. Spree.routes still
exists, we're just preferring to use Spree.pathFor per solidusio#3405.

It uses Proxy to issue a deprecation warning, however if Proxy isn't defined (in older
browsers like IE) will just return the routes without deprecation. Hopefully the developer
of the app will have something newer than IE, if they don't I imagine they have
bigger problems than JS route deprecations 😬

Also adds Proxy to ESlint so it doesn't trigger the no-undef rule.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@aldesantis @jarednorman @seand7565