Replace Spree.routes with Spree.pathFor

[seand7565]

THIS PR SPONSORED BY Super Good Software

Description
This PR replaces all instances of Spree.routes with Spree.pathFor. Spree.routes still
exists, we're just preferring to use Spree.pathFor per #3405.

I've added some lines about deprecation to the JS files where Spree.routes live. We should probably output a warning message to console on use, but I'm not 100% sure what the best way to go about it is, since it's just a variable. How do you console.warn when calling a variable in Javascript? 🤔 If there's a good way to do this, please let me know and I'll add it in.

Checklist:

• I have followed Pull Request guidelines
• I have added a detailed description into each commit message
• I have updated Guides and README accordingly to this change (if needed)
• I have added tests to cover this change (if needed)
• I have attached screenshots to this PR for visual changes (if needed)

[jarednorman]

Nice! I actually had a chat with Hawth about this.

[kennyadsl]

I like this but I think we should really deprecate accessing Spree.routes. I didn't try but I think we should change it into:

Spree.routes = function() {
  console.log("Spree.routes is deprecated, please use Spree.pathFor");

  {
    states_search: Spree.pathFor('api/states'),
    apply_coupon_code: function(order_id) {
      return Spree.pathFor("api/orders/" + order_id + "/coupon_codes");
    }
  }
};

But I'm not sure howbackend/app/assets/javascripts/spree/backend/routes.js will behave, and we should find a way to don't let those endpoints to emit the deprecation., so I think we need some extra code.

[tvdeyen]

This is great, thanks. A future iteration could add support for an params object that would allow for Spree.pathFor('api/shipments', { order_number: 'R1234567' }). Concatenating string seams a bit strange, but this is good for now and can be tackled in a subsequent PR.

Thanks @jarednorman for sponsoring 👏🏻

[aldesantis]

I think moving forward we may even want to use something such as https://github.com/railsware/js-routes which works really well.

@kennyadsl as for the deprecation, I don't think there's a way to do it, because your suggestion turns Spree.routes into a function, changing the API to call it (it would become Spree.routes()) and breaking backwards compatibility.

[kennyadsl]

I think this should do the trick:

const Spree.routes_deprecation_proxy = {
  get: function(obj, prop) {
    console.log("This is deprecated, please use ...");

    return obj[prop]
  },

  set: function(obj, prop, value) {
    console.log("This is deprecated, please use ...");

    obj[prop] = value
    return value;
  }
};

Spree.routes = new Proxy({
  states_search: Spree.pathFor('api/states'),
  apply_coupon_code: function(order_id) {
    return Spree.pathFor("api/orders/" + order_id + "/coupon_codes");
  }
}, Spree.routes_deprecation_proxy);

The only bad thing is that, when we add new routes in the backend, we need to create a new Spree.routes object, instead of adding properties to it. This is needed to avoid emitting deprecation warnings with our own code. Something like

// backend/app/assets/javascripts/spree/backend/routes.js

Spree.routes = new Proxy({
  states_search: Spree.pathFor('api/states'),
  apply_coupon_code: function(order_id) {
    return Spree.pathFor("api/orders/" + order_id + "/coupon_codes");
  },

  checkouts_api = Spree.pathFor('api/checkouts'),
  classifications_api = Spree.pathFor('api/classifications'),
  option_value_search = Spree.pathFor('api/option_values')
}, Spree.routes_deprecation_proxy);

but I don't think it's a big deal.

Thanks @dinohamzic for pointing out Proxy.

[seand7565]

@kennyadsl That's perfect! I had no idea about Proxy. I'll plug it in and push again later. Thanks!

[kennyadsl]

@seand7565 👍. Since that's a .js.erb file, we can even conditionally print the deprecation warning only if it's not in production.

[seand7565]

@kennyadsl Added the deprecation warnings, they work great! Thanks for the heads up about Proxy!

[kennyadsl]

I left 2 small things but we are almost there. Thanks @seand7565!

[jarednorman]

Other than @kennyadsl's comments, this also looks good to me. Nice call on the proxy.

[seand7565]

Hey all - the latest version should fix the hound violation - and also falls back to not using Proxy if the user is on IE11. If anyone has a windows machine and can test that out... I'd really appreciate it. 🙏

[aldesantis]

Would it be better to actually pass the environment to the JS code at runtime? I'm not a huge fan of mixing Ruby and JS like this, I think it could potentially generate a lot of confusion and subtle issues.

[seand7565]

I'm not quite sure how to go about this - do you have an example? We could assign a JS variable at the top of the file var production = <%= Rails.env == "production" %>, is that what you mean?

Also, I noticed in the frontend JS file, there's another deprecation done like this:

  if (console && console.warn) {
    console.warn('Spree.url is deprecated, and will be removed from a future Solidus version.');
  }

I hadn't noticed it before - would it be better to follow the already existing example set there?

Thanks!

[aldesantis]

@seand7565 100%, I didn't realize we had done it before. This will print the warning even in production, but IMO it's okay to be consistent and follow the existing pattern. If we want to only print the warnings in production, let's make sure we change it everywhere, and maybe in a separate PR.

@kennyadsl what do you think?

[kennyadsl]

Considering it only happens in the admin, I think it's not too bad if we print a deprecation warning in the console. So 👍 on making them more uniform.

For the other PR to use the Rais.env check, I guess the JS file in the frontend is not an .erb and I wouldn't add ruby to it since people can be including frontend files in unexpected ways (maybe with webpack?) and this would be a breaking change.

[aldesantis]

Thanks @seand7565! ❤️

[jarednorman]

Thank you @seand7565! 🙂