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

Add EnsureAssetsCompiled feature #222

Merged
merged 2 commits into from Jan 25, 2016

Conversation

@robwise
Copy link
Contributor

robwise commented Jan 24, 2016

Review on Reviewable

@robwise robwise force-pushed the ensure-assets-compiled branch from 45e0581 to e04314e Jan 24, 2016
@justin808

This comment has been minimized.

Copy link
Member

justin808 commented Jan 24, 2016

Looking good!


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


docs/additional_reading/rspec_configuration.md, line 2 [r1] (raw file):
If you did use stale Webpack assets, you will get invalid test results as your tests do not use the very latest JavaScript code.


docs/additional_reading/rspec_configuration.md, line 4 [r1] (raw file):
@samnang Please review. I'm pretty sure we need to pass config.


docs/additional_reading/rspec_configuration.md, line 6 [r1] (raw file):
and only if you do not have the webpack -w processes that would build the generated files.


docs/additional_reading/rspec_configuration.md, line 8 [r1] (raw file):
per my prior statement. I would not mention TDD workflow.

If you do not want to be slowed down by re-compiling webpack assets from scratch every test run, you can call ...

README.md, line 379 [r1] (raw file):
👍


Comments from the review on Reviewable.io

@robwise

This comment has been minimized.

Copy link
Contributor Author

robwise commented Jan 24, 2016

Review status: all files reviewed at latest revision, 4 unresolved discussions.


docs/additional_reading/rspec_configuration.md, line 2 [r1] (raw file):
Done.


docs/additional_reading/rspec_configuration.md, line 6 [r1] (raw file):
see below I address that next


docs/additional_reading/rspec_configuration.md, line 8 [r1] (raw file):
what prior statement?


Comments from the review on Reviewable.io

@robwise robwise force-pushed the ensure-assets-compiled branch from 2ecb065 to de0e680 Jan 25, 2016
@justin808

This comment has been minimized.

Copy link
Member

justin808 commented Jan 25, 2016

:lgtm_strong: 🎉 👏


Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from the review on Reviewable.io

justin808 added a commit that referenced this pull request Jan 25, 2016
Add EnsureAssetsCompiled feature
@justin808 justin808 merged commit 8c3604d into master Jan 25, 2016
2 of 4 checks passed
2 of 4 checks passed
coverage/coveralls Coverage decreased (-0.0%) to 91.94%
Details
code-review/reviewable Review in progress: all files reviewed, 1 unresolved discussion
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@robwise robwise deleted the ensure-assets-compiled branch Jan 25, 2016
@samnang

This comment has been minimized.

Copy link
Contributor

samnang commented Jan 26, 2016

docs/additional_reading/rspec_configuration.md, line 4 [r1] (raw file):
@justin808 @robwise Great stuff. I have few comments on this:

  1. instead of doing before(:example), we should use before(:suite).
  2. Instead letting users of our gem to know about the method and we need to expose that, we just have lib/react_on_rails/rspec file helper, so the user just require that file react_on_rails/rspec in their rails helper to have this integration helper.

Comments from the review on Reviewable.io

@robwise

This comment has been minimized.

Copy link
Contributor Author

robwise commented Jan 26, 2016

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


docs/additional_reading/rspec_configuration.md, line 4 [r1] (raw file):
@samnang

  1. If I do before suite, the thing will compile webpack bundles even if the user is just doing a quick unit test that has nothing to do with webpack.
  2. We need access to the RSpec config, so we have to have the user pass it as a variable. Also, there was an issue where if we have auto-executing code on require, then this runs twice (once on requiring of the gem by Rails, once on requiring in the rails helper)

Comments from the review on Reviewable.io

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.