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

Use turbolinks:before-visit, not turbolinks:before-cache #644

Merged
merged 1 commit into from
Dec 25, 2016

Conversation

volkanunsal
Copy link
Contributor

@volkanunsal volkanunsal commented Dec 11, 2016

This PR includes a page that renders a component when Turbolinks cache is disabled to test whether the component is really unmounted when the user navigates away from the page. The hook on turbolinks:before-visit will ensure that this will happen in all cases. The previous event hook turbolinks:before-cache was not called when the Turbolinks cache is disabled with a tag. That's why the erb page adds a meta tag to disable the cache.


This change is Reviewable

@volkanunsal
Copy link
Contributor Author

It looks like there are some Rubocop violations in files I haven't touched...

@justin808
Copy link
Member

This looks really good.

You seem to have update gems which is good. Please address the rubocop issues as well.

Great job!


Reviewed 9 of 9 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


spec/dummy/Gemfile, line 16 at r1 (raw file):

gem 'coffee-rails', '~> 4.1.0'
# See https://github.com/rails/execjs#readme for more supported runtimes
gem 'mini_racer', '= 0.1.7'

what does this PR have to do with forcing the mini_racer version?


spec/dummy/app/views/pages/turbolinks_cache_disabled.erb, line 10 at r1 (raw file):

<hr/>

This page demostrates that components are still unmounted when the Turbolinks cache is disabled.

demost => typo


Comments from Reviewable

@volkanunsal
Copy link
Contributor Author

volkanunsal commented Dec 22, 2016

Re: mini_racer version, I had a bit of a problem having the bundler install 0.1.7, instead of 0.1.4. I'm not positive why this was the case, but it wouldn't work for me without exact versioning. May not be a universal issue. I can take that out.

@justin808
Copy link
Member

@volkanunsal Let's get this to pass CI. I'd like to get this into the next release.

@volkanunsal
Copy link
Contributor Author

@justin808 I addressed the mini_racer issue. I'd rather not touch the rubocop issues because I didn't cause them and they seem rather opinionated from my point of view. If there is a base commit that fixes them, I can rebase from that.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.75% when pulling 5682963 on volkanunsal:629 into 6370447 on shakacode:master.

@justin808
Copy link
Member

Please rebase your changes on top of my latest commit.

@justin808
Copy link
Member

@volkanunsal Please create a changelog entry and please be consistent with the others.

@volkanunsal
Copy link
Contributor Author

@justin808 Bumping the version number up to 6.4.0. Is that cool?

@volkanunsal
Copy link
Contributor Author

Actually, it's probably a minor release now that I think of it. Bumping it up to 6.3.3. Here is the changelog entry:

[6.3.3] - 2016-12-25

  • By using the hook on turbolinks:before-visit to unmount the components, we ensure that components are unmounted even when Turbolinks cache is disabled. Previously, when Turbolinks cache was disabled, the component was not being unmounted because we used turbolinks:before-visit event hook.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.286% when pulling f571edd on volkanunsal:629 into 8bcaa34 on shakacode:master.

@justin808
Copy link
Member

@volkanunsal Please redo your last commit, and rebase on top of master. You pulled in lots of extraneous changes. Let's do 6.3.3, and please put the entry at the bottom of the changelog file that shows the diff.


Review status: 6 of 48 files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@volkanunsal
Copy link
Contributor Author

Ah, crap, yes, you're right. I think I merged before rebasing it.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.286% when pulling 8456c96 on volkanunsal:629 into 4c0f963 on shakacode:master.

@volkanunsal volkanunsal force-pushed the 629 branch 2 times, most recently from 94c7e74 to 79b4b32 Compare December 25, 2016 22:03
@justin808
Copy link
Member

Reviewed 3 of 4 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


CHANGELOG.md, line 11 at r3 (raw file):

## [6.3.3] - 2016-12-25
- By using the hook on `turbolinks:before-visit` to unmount the components, we can ensure that components are unmounted even when Turbolinks cache is disabled. Previously, we used `turbolinks:before-cache` event hook.

Please reference the PR and the author like the other PRs below.


spec/dummy/Gemfile, line 16 at r2 (raw file):

gem 'coffee-rails', '~> 4.1.0'
# See https://github.com/rails/execjs#readme for more supported runtimes
# gem 'mini_racer', platforms: :ruby

Why is removing this part of the PR? It should not be.

If you believe that we should remove mini_racer, that should be a different PR.


Comments from Reviewable

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.286% when pulling 79b4b32 on volkanunsal:629 into 4c0f963 on shakacode:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.286% when pulling 79b4b32 on volkanunsal:629 into 4c0f963 on shakacode:master.

@volkanunsal
Copy link
Contributor Author

@justin808 I think there is another mini_racer below that one. That was already commented out in the original, if I recall correctly.

@volkanunsal
Copy link
Contributor Author

@justin808 Took out the mini_racer in the comment.

@justin808
Copy link
Member

@volkanunsal Thanks for pointing out the min-racer thing.

@justin808
Copy link
Member

Review status: 7 of 9 files reviewed at latest revision, 5 unresolved discussions.


CHANGELOG.md, line 10 at r4 (raw file):

## [6.3.3] - 2016-12-25
- By using the hook on `turbolinks:before-visit` to unmount the components, we can ensure that components are unmounted even when Turbolinks cache is disabled. Previously, we used `turbolinks:before-cache` event hook. [#644](https://github.com/shakacode/react_on_rails/pull/644) by [volkanunsal](https://github.com/volkanunsal)

period at end.


Comments from Reviewable

@justin808
Copy link
Member

Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 5 unresolved discussions.


Comments from Reviewable

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.286% when pulling 0878b35 on volkanunsal:629 into 4c0f963 on shakacode:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.286% when pulling 0878b35 on volkanunsal:629 into 4c0f963 on shakacode:master.

@justin808
Copy link
Member

:lgtm:

thanks! great job!


Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 5 unresolved discussions.


Comments from Reviewable

@justin808 justin808 merged commit ccd970a into shakacode:master Dec 25, 2016
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