Improve EnsureAssetsCompiled logic to wait for webpack process to finish

[robwise]

Because there may be situations where a user is running a webpack
process in watch mode in the background that will recompile the
assets, it is possible that if the build time is extremely long,
the user could start their test suite before the build completes. In this case,
EnsureAssetsCompiled would start compiling the assets again even
though there is already a webpack process doing that.

This commit adds behavior to EnsureAssetsCompiled to check whether
or not the webpack process is running in the background, and, if so, will
defer the responsibility of compilation to that process, re-checking
the compiled assets every second until they are up to date.

[justin808]

Looking great!

---

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

---

lib/react_on_rails/ensure_assets_compiled.rb, line 56 [r1] (raw file):
Please display a message how to do this more efficient, by running blah, blah, blah, etc. or a doc link.

---

lib/react_on_rails/ensure_assets_compiled.rb, line 63 [r1] (raw file):
It will be confusing if one or the other is not running. I'd recommend we error out in the case of we detect one but not the other. FAIL FAST!

---

lib/react_on_rails/ensure_assets_compiled.rb, line 72 [r1] (raw file):
Did you test this?
It's (-w|--watch)

-watch is not correct

---

lib/react_on_rails/utils.rb, line 14 [r1] (raw file):
This is a global.

IDEALLY, we should run both client and server concurrently, rather than sequentially, but you can defer on that one.

Where are you capturing the results? and aborting if not success?

---

Comments from the review on Reviewable.io

[robwise]

Reviewed 2 of 6 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

lib/react_on_rails/ensure_assets_compiled.rb, line 56 [r1] (raw file):
Done.

---

lib/react_on_rails/ensure_assets_compiled.rb, line 63 [r1] (raw file):
Done.

---

lib/react_on_rails/ensure_assets_compiled.rb, line 72 [r1] (raw file):
Tested in one of those regex websites as well as the spec dummy (which uses just the -w so I forgot about the double dash when passing the long form).

Done.

---

lib/react_on_rails/utils.rb, line 14 [r1] (raw file):
WebpackAssetsCompiler#compile: https://github.com/shakacode/react_on_rails/blob/master/lib/react_on_rails/ensure_assets_compiled.rb#L36

---

Comments from the review on Reviewable.io

[robwise]

Reviewed 4 of 6 files at r1.
Review status: 4 of 6 files reviewed at latest revision, all discussions resolved.

---

Comments from the review on Reviewable.io

[justin808]

More great code. Couple minor comments.

---

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

---

lib/react_on_rails/ensure_assets_compiled.rb, line 57 [r2] (raw file):
Too vague. Show what you are matching. Plus a doc link!

---

lib/react_on_rails/ensure_assets_compiled.rb, line 79 [r2] (raw file):
👍

---

lib/react_on_rails/utils.rb, line 14 [r1] (raw file):
Are we sure that we can rely on the content having error. Sometimes we have files with the word error in the file name. I prefer the exit code if that is reliable for knowing we failed. Or if we are SURE, then this is OK.

---

Comments from the review on Reviewable.io

[robwise]

Reviewed 2 of 3 files at r2.
Review status: 5 of 6 files reviewed at latest revision, 2 unresolved discussions.

---

lib/react_on_rails/ensure_assets_compiled.rb, line 57 [r2] (raw file):
Okay the link to the docs was a little long, so check how I did it. I can change if you want the full-length link in there. It's this, unless I can shorten it somehow but I can't remember?: https://github.com/shakacode/react_on_rails/blob/master/docs/additional_reading/rspec_configuration.md

I'm not sure what you meant by matching here though, I'm not matching anything. This is just the output that happens after we compile the bundle.

---

lib/react_on_rails/utils.rb, line 14 [r1] (raw file):
@justin808 wait I'm confused, you originally wrote it so it was grepping for the word "error," but my commit changes that to use the exit status. That link is to the current implementation of WebpackAssetsCompiler#compile on master, the new implementation uses this helper I wrote here.

---

Comments from the review on Reviewable.io

[justin808]

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

lib/react_on_rails/ensure_assets_compiled.rb, line 57 [r2] (raw file):
You need the full URL. Yes, it's long. It's worth it.

---

lib/react_on_rails/utils.rb, line 14 [r1] (raw file):
OK -- so long as you can display the error when you exit due to this.

---

Comments from the review on Reviewable.io

[robwise]

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

---

lib/react_on_rails/ensure_assets_compiled.rb, line 57 [r2] (raw file):
Done.

---

lib/react_on_rails/utils.rb, line 14 [r1] (raw file):
@justin808 Yep, this method is the best of both worlds in this sense. Here is the link to the same place but how I have it in this PR: https://github.com/shakacode/react_on_rails/blob/rob/improve-ensure-assets-compiled/lib/react_on_rails/ensure_assets_compiled.rb#L54

Notice I'm still capturing the output of the process, then I check the exit code, and if the exit code is a failure, then I log an error message that includes the captured output so they know what the error was.

---

Comments from the review on Reviewable.io

[robwise]

Reviewed 1 of 3 files at r2, 1 of 1 files at r3.
Review status: 5 of 6 files reviewed at latest revision, all discussions resolved.

---

Comments from the review on Reviewable.io

[justin808]

Amazing job @robwise!

---

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from the review on Reviewable.io

[justin808]

Please merge, ensure changelog updated, docs ready etc. and let me know if I should release later today.

---

Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from the review on Reviewable.io

[robwise]

Latest code makes it so we only run once per suite run (assuming the suite has a feature test in it) instead of running every feature test.

Also, the config now allows for specifying where webpack is outputting generated files. By default, this is app/assets/javascripts/generated and app/assets/stylesheets/generated so that we don't break any current builds that are following the convention of the generators.

Lastly, we were using unix file separators in the configuration which I changed to be platform-dependent.

@justin808 I think I should just update the docs in PR 252 to include this information, right?

---

Reviewed 3 of 4 files at r5, 1 of 1 files at r6.
Review status: 6 of 7 files reviewed at latest revision, 2 unresolved discussions.

---

lib/react_on_rails/ensure_assets_compiled.rb, line 5 [r5] (raw file):
I had to do it this way because linter doesn't like it when I use @@. Supposedly that's a bad practice because subclasses have access to the variable. Whatevs

---

lib/react_on_rails/webpack_assets_status_checker.rb, line 14 [r5] (raw file):
This was only temporarily necessary, pls ignore

---

Comments from the review on Reviewable.io

[justin808]

Review status: 6 of 7 files reviewed at latest revision, 3 unresolved discussions.

---

lib/react_on_rails/ensure_assets_compiled.rb, line 5 [r5] (raw file):
Correct.

---

lib/react_on_rails/ensure_assets_compiled.rb, line 44 [r6] (raw file):
I personally think we should have one class per file.

---

Comments from the review on Reviewable.io

[justin808]

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

---

lib/react_on_rails/ensure_assets_compiled.rb, line 16 [r6] (raw file):
Is there a return value that is useful from this call? I don't see anything useful other than to call call from this created object.

Just use a static calls

You can make this one API:

ReactOnRails::EnsureAssetsCompiled.call(client_assets_checker:, server_assets_checker:, process_checker:)

---

lib/react_on_rails/ensure_assets_compiled.rb, line 21 [r6] (raw file):
This API is really awkward.

---

lib/react_on_rails/ensure_assets_compiled.rb, line 21 [r6] (raw file):
we don't need this method -- just use static calls

---

lib/react_on_rails/ensure_assets_compiled.rb, line 27 [r6] (raw file):
Make this method static

return if @has_been_run

---

Comments from the review on Reviewable.io

[justin808]

A couple comments.

---

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

---

Comments from the review on Reviewable.io

[robwise]

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

---

lib/react_on_rails/ensure_assets_compiled.rb, line 16 [r6] (raw file):
@samnang what's your thoughts here? I was trying to follow the style you taught me when pairing, but maybe this is a different case?

---

lib/react_on_rails/ensure_assets_compiled.rb, line 21 [r6] (raw file):
@justin808 suggestions?

---

lib/react_on_rails/ensure_assets_compiled.rb, line 21 [r6] (raw file):
@justin808 I need DI for tests tho

---

lib/react_on_rails/ensure_assets_compiled.rb, line 44 [r6] (raw file):
@samnang what is the convention in Ruby if I'm creating a utility class that is only ever used by this other class

---

Comments from the review on Reviewable.io

[robwise]

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

---

lib/react_on_rails/ensure_assets_compiled.rb, line 27 [r6] (raw file):
I have instance-level dependencies in this method in the form of webpack_process_checker and webpack_assets_checker

---

Comments from the review on Reviewable.io

[justin808]

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

---

lib/react_on_rails/ensure_assets_compiled.rb, line 21 [r6] (raw file):
Please don't ever sacrifice the end code API for tests.

---

lib/react_on_rails/ensure_assets_compiled.rb, line 27 [r6] (raw file):
You can pass those, in just have defaults.

---

lib/react_on_rails/ensure_assets_compiled.rb, line 44 [r6] (raw file):
You can/should test the other classes. I don't see why you'd have multiple classes in one file.

---

Comments from the review on Reviewable.io

[robwise]

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

---

lib/react_on_rails/ensure_assets_compiled.rb, line 44 [r6] (raw file):
I can move the classes to separate files, I was just following what I was shown. I have extracted these other classes specifically because they are simple wrappers on third-party libraries/applications. This isolates dependencies on webpack and running webpack processes as well as using the pgrep. To test these would just be testing the unix pgrep utility or webpack themselves.

---

Comments from the review on Reviewable.io

[samnang]

lib/react_on_rails/ensure_assets_compiled.rb, line 44 [r6] (raw file):
If it's very simple class and ever used as their internal functionality, it's okay to leave it there, but if it's not simple, better to have it in another file, but it still in the namespace under it.

---

Comments from the review on Reviewable.io

[samnang]

lib/react_on_rails/ensure_assets_compiled.rb, line 16 [r6] (raw file):
In this case, you could inline here. The benefit to use dependency injection in constructor, so you could have more than one public methods, and extract some functionality in private methods. For static methods, if you want to extract methods, you have to always pass parameters, and then we don't get any benefit from encapsulation.

---

Comments from the review on Reviewable.io

[justin808]

Let's merge if this passes CI.

[robwise]

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

---

Comments from the review on Reviewable.io

[justin808]

BOOM 🎉