Skip to content

SSR fixes #1252

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

Closed
wants to merge 30 commits into from
Closed

SSR fixes #1252

wants to merge 30 commits into from

Conversation

Judahmeek
Copy link
Collaborator

@Judahmeek Judahmeek commented Feb 13, 2023

Superseded by #1274

@Judahmeek Judahmeek changed the title WIP SSR fixes Feb 13, 2023
@justin808
Copy link
Collaborator

@Judahmeek is this ready?

@@ -10,6 +10,7 @@ class ExecJSRenderer

def initialize(options={})
js_code = options[:code] || raise('Pass `code:` option to instantiate a JS context!')
# File.write("./tmp/latest_js_context.js", js_code, mode: "w+")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Judahmeek remove comment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather leave it there & then convert it into a proper trace log statement later on.

@@ -11,6 +11,8 @@ def test_it_loads_JS_from_the_webpacker_container
WebpackerHelpers.compile
container = React::ServerRendering::WebpackerManifestContainer.new
assert_not_empty container.find_asset('application.js')
container = React::ServerRendering::SeparateServerBundleContainer.new
assert_not_empty container.find_asset('server_rendering.js')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this test work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It really doesn't do much more than confirm that React::ServerRendering::SeparateServerBundleContainer.new.find_asset('server_rendering.js') doesn't throw an error.

But that's the same level of testing for React::ServerRendering::WebpackerManifestContainer that was already there so I'm simply applying the same standards.

@RiccardoMargiotta
Copy link
Contributor

I can help test this if you can provide a bit more context around the issue. I already have SSR working with Shakapacker 6.5.5, but I do have the fix in place from Ryan's suggestion in #970 for splitChunks usage. Is this the same thing, or something different?

@Judahmeek
Copy link
Collaborator Author

@RiccardoMargiotta these changes have been tested against https://github.com/ahangarha/react-rails-with-shakapacker-for-ssr/tree/ssr-from-react_on_rail-dummy

As you'll see if you examine that repository, these changes are meant for a webpacker configuration in which the client & server bundles are compiled separately so the server bundle is no longer in the webpacker manifest.

@ahangarha
Copy link
Collaborator

@RiccardoMargiotta Do you have any public repo demonstrating the solution? I tried to apply the mentioned solution but couldn't make SSR work. It would be great to share to compare the performance and complexity of the two solutions.

@RiccardoMargiotta
Copy link
Contributor

@ahangarha Nothing public, unfortunately. I'm not quite sure what I've been doing differently, although the main difference is probably that I don't make use of webpack-dev-server. I'll try to see if I can create a test app matching my setup.

@Judahmeek Judahmeek marked this pull request as ready for review February 23, 2023 03:24
@Judahmeek Judahmeek requested review from justin808 and ahangarha and removed request for justin808 February 23, 2023 03:24
@Judahmeek
Copy link
Collaborator Author

@ahangarha please let me know when you'll be able to complete the documentation for this PR or if you would prefer that I write it up.

@ahangarha
Copy link
Collaborator

@ahangarha please let me know when you'll be able to complete the documentation for this PR or if you would prefer that I write it up.

@Judahmeek I need to finish a task by Friday. After that, this is my priority.

@@ -1,3 +1,5 @@
const { environment } = require('@rails/webpacker')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why support webpacker 3? it's too old!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because decisions on what to support are above our pay-grade. 😂

@@ -102,6 +103,8 @@ def assets_precompiled?
def asset_container_class
if self.class.asset_container_class.present?
self.class.asset_container_class
elsif SeparateServerBundleContainer.compatible? && self.class.dont_use_webpacker_manifest
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont use webpacker manifest should be the ONLY case for SSR

using the webpack dev server for the server bundle is a flawed approach

I originally supported this for React on Rails, and that was a mistake.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove dummy_webpacker4

@justin808
Copy link
Collaborator

@Judahmeek is this ready?

@@ -6,39 +6,28 @@
# For more information see: https://github.com/marketplace/actions/setup-ruby-jruby-and-truffleruby

name: Ruby
on: [push, pull_request]
on: [push]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

running on push & pull_request was redundant

@justin808
Copy link
Collaborator

@Judahmeek we want to get this merged.

@Judahmeek Judahmeek closed this May 18, 2023
@Judahmeek
Copy link
Collaborator Author

Superseded by #1274

@Judahmeek Judahmeek deleted the judahmeek/ssr branch June 30, 2023 00:41
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.

4 participants