remove extra js eval call

[wyattades]

Summary

The current implementation calls ExecJS::Runtime#eval twice during every react server-side render, but we can refactor it to only call eval once.

This causes a 2x speedup (e.g. 800ms -> 400ms) with the Node.js runtime, and other runtimes with slow IO.

Pull Request checklist

• Add/update test to cover these changes
• Update documentation
• Update CHANGELOG file

Other Information

---

This change is 

[wyattades]

alternatively, we could move this into serverRenderReactComponent. I'll let you all decide

[justin808]

This is a good solution.

[justin808]

BTW, @Judahmeek @ahangarha @KhaledEmaraDev This line would be a great place to add a configurable cleanup code for SSR. For example, in the past, I saw weird side effects from MobX on SSR.

[justin808]

@wyattades I see your point that we could move console.history = []; to here: https://github.com/shakacode/react_on_rails/blob/master/node_package/src/serverRenderReactComponent.ts#L12

One can argue that it makes sense to keep the generated JS as small as possible, so I like that solution.

@Judahmeek @alexeyr-ci what do you guys think?

I vote we:

1. rename the current function serverRenderReactComponent to something like serverRenderReactComponentInternal and not export it.
2. Create serverRenderReactComponent in the same file with the try/finally block.

[alexeyr-ci]

If nobody calls serverRenderReactComponentInternal other than serverRenderReactComponent, why bother creating it (instead of just adding try/finally there directly)? If someone does, sure.

[justin808]

Easier to read. Function is already too long.

[justin808]

Awesome work @wyattades. Can you document how you profiled this? Maybe you could add such information to CONTRIBUTING.md?

[justin808]

Good work.

To merge, I'd like to see:

1. Tests for the changes if easy to do.
2. We need to deprecate or prevent eval_js from being called directly, as it won't clear the console.history any more.
3. We need a changelog.md entry.

[justin808]

This is a good solution.

[wyattades]

Tests for the changes if easy to do.
We need a changelog.md entry.

Can do, probably by next week.

Can you document how you profiled this? Maybe you could add such information to CONTRIBUTING.md?

I don't have anything reproducible at the moment, since I tested it against a private project. But this would be as simple as running a benchmark on request durations on the dummy project. Maybe it'd be good to make a bin/benchmark script that tests a few different execjs runtimes and React SSR scenarios?

We need to deprecate or prevent eval_js

I don't see any documentation that it's public, do we need deprecation?

[justin808]

@wyattades

I don't see any documentation that it's public, do we need deprecation?
Correct! we don't!

[alexeyr-ci]

I checked if we have similar problems elsewhere and I don't think so (at least with eval calls). There is

react_on_rails/spec/dummy/spec/system/integration_spec.rb

Lines 16 to 24 in 43efef2

	def wait_for_ajax
	Timeout.timeout(Capybara.default_max_wait_time) do
	loop until finished_all_ajax_requests?
	end
	end
	def finished_all_ajax_requests?
	page.evaluate_script("jQuery.active").zero?
	end

but it's in tests.

[Judahmeek]

@wyattades can you also rebase on master?

CI won't pass until you do since we had some of the jobs set to trigger on push only, which meant that they didn't trigger for PRs from forks.

This oversight was corrected by #1553

[wyattades]

Can you document how you profiled this? Maybe you could add such information to CONTRIBUTING.md?

Made a separate PR for this, since I ran into some issues with the dummy app #1555

[justin808]

Prefer to put formatting in separate PR.

[justin808]

Nice and small update!