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

Emit Spree.url deprecation warning in all envs #2017

Merged
merged 3 commits into from
Jun 15, 2017

Conversation

jhawthorn
Copy link
Contributor

This has been deprecated forever, but I'm still not confident enough to remove it. It's possible that it hasn't been seen by users because Spree.env has been unset (as it is in our default frontend).

This commit changes the warning to be issued always (so long as console and console.warn are defined).

This also removes a call to Spree.url from solidus_frontend.

This has been deprecated forever, but I'm still not confident enough to
remove it. It's possible that it hasn't been seen by users because
Spree.env has been unset (as it is in our default frontend).

This commit changes the warning to be issued always (so long as console
and console.warn are defined).
Previously we were sending our data as a query string, instead we should
send it as JSON, since this is a PUT request. This also avoids using the
deprecated Spree.url
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Fine.

We should maybe also change the warning message. Often Spree.url is used only to get a URL, like in the example above. So, adding "use Spree.routes to get URLs" could help developers understand what is wrong.

@jhawthorn
Copy link
Contributor Author

We should maybe also change the warning message. Often Spree.url is used only to get a URL, like in the example above. So, adding "use Spree.routes to get URLs" could help developers understand what is wrong.

I don't think Spree.routes provides the functionality Spree.url has either. I think I will just drop the suggestion from the warning.

This suggestion could be confusing because Spree.ajax isn't a direct
replacement of Spree.url
@jhawthorn jhawthorn merged commit cf9eebd into solidusio:master Jun 15, 2017
@jhawthorn jhawthorn deleted the stronger_spree_url_deprecation branch June 15, 2017 21:59
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.

3 participants