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

Remote form and AJAX request redirects never run events #152

Open
jclusso opened this issue Mar 6, 2020 · 2 comments
Open

Remote form and AJAX request redirects never run events #152

jclusso opened this issue Mar 6, 2020 · 2 comments

Comments

@jclusso
Copy link

jclusso commented Mar 6, 2020

I spent a lot of time reviewing the code and it's quite confusing to me why the Rack::Tracker#call method does this swap of moving the tracker hash in and out of the session. Because of this, if the request is not HTML, the tracker never gets stored in the session. Another question I have is why not just store the tracker hash in the session from the start? This swapping from env['tracker'] to the session['tracker'] just seems overly complex and I can't understand the purpose.

From what I can tell this seems to be the cause that remote forms and any AJAX request that redirects never get the events executed in the next HTML request. Since the Rack::Tracker#call method returns unless the request is HTML the env['tracker'] never moves into the session['tracker']. This means the events are just lost.

It appears #79 brings this issue up and #133 brings up a possible solution. #133 would be the most ideal solution but it's likely a pain and requires additional templates for every handler.

Right now I'm working on making this work using just the session and I'll submit a pull request. Just wanted to get the discussion started on this since I'm not sure why the session wasn't always just directly used.

@DonSchado
Copy link
Collaborator

Hi @jclusso!
Thanks a lot for taking the effort.
To be honest, I don't know exactly "why", but this library evolved from a former google analytics integration, trying to unify all the snippets into one place.
I guess the architectural decisions just went over here without big reconsiderations.

But nonetheless thanks for challenging that.
We will try to review the PR as soon as possible.

Could you already run this change in a production environment?

@jclusso
Copy link
Author

jclusso commented Mar 25, 2020

Hey @DonSchado, it’s being used in production already. I think I fully figured out how everything works and I got the tests to pass. I wouldn’t say I’m 100% confident until someone that has some more experience with this library does a review.

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

3 participants