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

Split TG Into JS Lib and Rails Engine Parts #166

Open
marutypes opened this issue Oct 20, 2016 · 0 comments
Open

Split TG Into JS Lib and Rails Engine Parts #166

marutypes opened this issue Oct 20, 2016 · 0 comments

Comments

@marutypes
Copy link
Contributor

marutypes commented Oct 20, 2016

Splitting Up The Family

As part of my work with making a service worker that interacts with TG, I've been interacting with it in new and exciting ways. One of these exposed some strange interactions with redirects and the current full page navigation behaviour. We've also put a tonne of collective time into building and testing a lot of the redirect logic both on the client and server side, as well as working around some of the quirks of the way Remote talks to Page and thus to Turbolinks. I've been gradually coming to the opinion that I'd like to see the JS decoupled from the server side components, allowing the library to live as a js module and be usable for projects that don't have access to rails, and the ruby code to live as a gem that merely depends on the js code and provides enhancements and helpers on top of it.

The server code is doing the following things as far as I can tell:

  • Providing rails helpers for interacting with TG or doing redirects using TG directly
  • Ensuring TG requests to external domains aren't allowed
  • Does some stuff with cookies to stop it from initializing on a non-GET
  • Attaching custom redirect headers
  • Attaching custom X-XHR-Referer headers

So lets go through them one by one and discuss whether they are core to the functionality of the library, could be replicated on the client side only, or could be made optional for users of the rails lib.

Providing Rails Helpers

There are at least two .rb files aimed at providing rails utilities or altering rails specific helper behaviour. These seem pretty appropriate to live in a companion helper gem, and the js library certainly does not need them to function.
verdict: No brainer

Ensuring TG requests to external domains aren't allowed

This one is a little weird to me. TG is already doing this on the client side, and would force a full page refresh if one of these went through. I guess it's just extra security? Seems fine to live in an optional helper gem alongside rails helpers.
verdict: Seems fine

Does some stuff with cookies to stop it from initializing on a non-GET

This one's really weird to me as well. Is the intention here that if you accidentally initiate your session on a POST request stuff could get weird so we prevent it? My thoughts on this are the same as ensuring requests to external domains aren't allowed. Seems optional / maybe a best practice. Don't need to force a dependency on this behaviour.
verdict: Seems fine

Attaching custom X-XHR-Redirected-To headers

Ah, this one is interesting. My PR for adding responseURL comparisons actually covers this, if not for the fact that Remote is currently abstaining from passing the url it's made a request to along to Turbolinks. This is definitely fixable, though a bit of a refactor. I don't see a reason to depend on sessions for this, it works fine but seems a bit hacky.
verdict: Already mostly done after my current PR, just need to refactor Remote a bit.

Attaching custom X-XHR-Referer headers

This one is the wildcard for me. Do we do this so that we can differentiate requests? It seems like a lot of code is dedicated to avoiding using the native Referer header. Is there a good reason for this? Is this overly defensive? Can we just make this opt-in?
** Edit: Pretty sure this header is basically there so that we can make rails' link_to 'back' :back work with TG**
verdict: I think this is totally doable, but I might be missing something here

Overall I think that removing dependencies of the JS on the server seems quite doable. I do think it would require probably a week of dedicated dev time and quite a bit of tophatting. I'm opening this partially just as a conversation starter, but I think it would be awesome if we could make it happen.

@Shopify/tnt

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

No branches or pull requests

1 participant