Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

SSR Fixes and better tests for v3.0.0 #1274

Merged
merged 9 commits into from
May 25, 2023
Merged

SSR Fixes and better tests for v3.0.0 #1274

merged 9 commits into from
May 25, 2023

Conversation

Judahmeek
Copy link
Collaborator

@Judahmeek Judahmeek commented May 14, 2023 •
edited by ahangarha
Loading

Summary

  • Remove support & testing for Webpacker 3/4.
  • Add support & testing for Shakapacker v6
  • Requires separate compilations for server & client bundles (see Webpack config)
  • Replaces WebpackManifestContainer, which searched for assets in the webpack manifest, with SeparateServerBundleContainer, which expects a single server bundle file & does not use the webpack manifest at all.

Pull Request checklist

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

@Judahmeek Judahmeek force-pushed the judahmeek/workingCI branch 13 times, most recently from 90dc16d to e5e1c99 Compare May 17, 2023 23:50
@Judahmeek Judahmeek requested review from ahangarha and justin808 May 18, 2023 03:16
@Judahmeek Judahmeek mentioned this pull request May 18, 2023
Closed
ahangarha
ahangarha reviewed May 18, 2023
View reviewed changes
Copy link
Collaborator

@ahangarha ahangarha left a comment

Choose a reason for hiding this comment

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

Very extensive PR 🙂

It would be great if we could break it into two PRs

  • Fix CI and the issue with requiring react-dom/client -> 2.7.1
  • SSR fix -> 2.8 or 3.0

rakelib/create_release.rake Outdated
@@ -39,7 +39,7 @@ task :create_release, %i[gem_version dry_run] do |_t, args|
puts 'Updating ujs:update'
Rake::Task['ujs:update'].invoke

Release.commit_the_changes('Update pre-bundled react and React ujs') unless is_dry_run
Copy link
Collaborator

Choose a reason for hiding this comment

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

Then the commit_the_changes method should be removed

.github/workflows/ruby.yml Outdated
Comment on lines 26 to 28
- run: bundle exec rake react:update
- run: bundle exec rake ujs:update
- run: ./check_for_uncommitted_files.sh
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can follow the DRY principle and create a rake task for this check. We can use it here and also in the release task.

@Judahmeek Judahmeek force-pushed the judahmeek/workingCI branch from f75f915 to 0a369c0 Compare May 18, 2023 17:06
justin808
justin808 reviewed May 19, 2023
View reviewed changes
Copy link
Collaborator

@justin808 justin808 left a comment

Choose a reason for hiding this comment

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

Looking good.

Let's get PR 1278 merged and fix the conflicts.

Appraisals Outdated
end

appraise 'rails-5.2_no_sprockets_webpacker_3' do
appraise 'rails-5.2_no_sprockets_shakapacker' do
gem 'rails', '~> 5.2.x'
Copy link
Collaborator

Choose a reason for hiding this comment

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

how much longer should we support rails 5.x?

Rails 5.2.1 Ended 11 months ago, (01 Jun 2022)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I recommend making a yearly plan for our support for different versions of Ruby, Rails, Node, and React. Then update documentation and CI in all of our projects based on that.

It should be done in a (maximum) 30 min call with key senior developers in the team.

test/support/webpacker_helpers.rb

module_function
def available?
defined?(Webpacker)
!!defined?(Webpacker)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we also add Shakapacker to support v7+?

test/support/webpacker_helpers.rb
Rake::Task['webpacker:compile'].reenable
Rake::Task['webpacker:compile'].invoke
end
Rake::Task['webpacker:compile'].reenable
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this change?

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 removed capture_io

justin808
justin808 reviewed May 19, 2023
View reviewed changes
Copy link
Collaborator

@justin808 justin808 left a comment

Choose a reason for hiding this comment

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

Looking good.

Let's get PR 1278 merged and fix the conflicts.

@justin808 justin808 changed the title SSR Fixes (Alternate Approach) SSR Fixes and better tests for v3.0.0 May 20, 2023
@Judahmeek Judahmeek force-pushed the judahmeek/workingCI branch from 0a369c0 to c093961 Compare May 20, 2023 06:28
@Judahmeek Judahmeek force-pushed the judahmeek/workingCI branch from c093961 to b321bfe Compare May 20, 2023 20:45
justin808
justin808 approved these changes May 21, 2023
View reviewed changes
Copy link
Collaborator

@justin808 justin808 left a comment

Choose a reason for hiding this comment

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

Looks ready. One debug comment left accidentally?

Do we need to update the README? and CHANGELOG? and release v3? Any upgrade guide?

CC: @ahangarha @Judahmeek

lib/react/server_rendering/exec_js_renderer.rb
@@ -10,7 +10,9 @@ class ExecJSRenderer

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

Choose a reason for hiding this comment

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

@Judahmeek keep this 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 had to use it to debug two different SSR error with ExecJS so yes, I'd like to keep it.

lib/react/server_rendering/separate_server_bundle_container.rb
class SeparateServerBundleContainer

def self.compatible?
!!defined?(Webpacker)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Check for Shakapacker?

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 don't want to try supporting Shakapacker v6 & v7 at the same time.

@Judahmeek Judahmeek merged commit 8c491af into master May 25, 2023
@Judahmeek Judahmeek deleted the judahmeek/workingCI branch May 25, 2023 21:44
@ahangarha ahangarha mentioned this pull request Jun 21, 2023
Closed
ahangarha pushed a commit that referenced this pull request Jul 3, 2023
@Judahmeek @ahangarha
SSR Fixes and better tests for v3.0.0 (#1274)
f7a612b 
* Fully support Shakapacker, including tests & SSR
@Judahmeek Judahmeek mentioned this pull request Jul 13, 2023
Closed
okuramasafumi added a commit to okuramasafumi/react-rails that referenced this pull request Oct 18, 2023
@okuramasafumi
Update README.md
ab5532f 
https://github.com/reactjs/react-rails/blob/master/CHANGELOG.md

> If using Webpacker/Shakapacker, requires upgrading to Shakapacker v7 reactjs#1274 and reactjs#1285

It looks like react-rails v3 supports Shakapacker v7 only.
@okuramasafumi okuramasafumi mentioned this pull request Oct 18, 2023
Merged
3 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@ahangarha ahangarha ahangarha left review comments

@justin808 justin808 justin808 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Judahmeek @justin808 @ahangarha