react-rails icon indicating copy to clipboard operation
react-rails copied to clipboard

Add support for multiple `require.context` with addition of `useContexts`

Open cymen opened this issue 4 years ago • 2 comments

Summary

In some cases, it is desirable to be able to have multiple require.context and attempt to resolve in each before falling back to global. This PR adds useContexts (in the pattern of useContext) which accepts an array of require.context and attempts to resolve the component/module in each context before falling back to global.

Other Information

I was not able to get the test suite to run so I have not been able to add tests yet.

cymen avatar Sep 03 '21 21:09 cymen

Thanks @cymen, this looks really promising for the way I'm trying to split JS bundles by route, but still use server-side rendering. I ran into the same problem as https://github.com/reactjs/react-rails/issues/1034 where I couldn't use multiple files for SSR, even when I disabled sourcemaps.

Here's a small example from my current setup, with a number of smaller split bundles. At the moment, because of the lack of SSR across all entry points, I've only split out components that I'm happy to be client-side rendered (prerender: false).

Before

server_rendering.js

var componentRequireContext = require.context('components', true);
var ReactRailsUJS = require('react_ujs');
ReactRailsUJS.useContext(componentRequireContext);

enquiry.js

var componentRequireContext = require.context('enquiry', true);
var ReactRailsUJS = require('react_ujs');
ReactRailsUJS.useContext(componentRequireContext);

search.js

var componentRequireContext = require.context('search', true);
var ReactRailsUJS = require('react_ujs');
ReactRailsUJS.useContext(componentRequireContext);

After

server_rendering.js

var componentsRequireContext = require.context('components', true);
var enquiryRequireContext = require.context('enquiry', true);
var searchRequireContext = require.context('search', true);

var ReactRailsUJS = require('react_ujs');
ReactRailsUJS.useContexts([
  componentsRequireContext,
  enquiryRequireContext,
  searchRequireContext
]);

No changes to enquiry or search.


I built a version of the package with your code, and I'm happy to say this lead to components from the enquiry and search entry points being bundled into server_rendering.js, allowing me to switch components from those bundles back to SSR (prerender: true). I was also able to run the test suite, and it doesn't seem like your changes have broken any of the existing tests. 👍

Hey @BookOfGreg, hope you're doing OK! This is a pretty useful addition. 😃

RiccardoMargiotta avatar Jan 07 '22 16:01 RiccardoMargiotta

@cymen, thanks for your contribution. Sorry for the delays to get this merged and shipped. Looks useful!

Please:

  1. rebase on master
  2. add unit tests
  3. add a CHANGELOG.md entry
  4. improve the clarity of the docs

Thanks!

justin808 avatar Sep 09 '22 09:09 justin808

@cymen, Do you have the availability to work on the changes suggested by @justin808? Please resolve the conflicts.

@cymen, thanks for your contribution. Sorry for the delays to get this merged and shipped. Looks useful!

Please:

  1. rebase on master
  2. add unit tests
  3. add a CHANGELOG.md entry
  4. improve the clarity of the docs

Thanks!

alkesh26 avatar Oct 07 '22 11:10 alkesh26

@RiccardoMargiotta do you have time to push a new PR that includes your suggestions?

justin808 avatar Nov 26 '22 04:11 justin808

@justin808 I've created https://github.com/reactjs/react-rails/pull/1221. Could you run the workflow please?

RiccardoMargiotta avatar Nov 28 '22 12:11 RiccardoMargiotta

@RiccardoMargiotta Thanks for pushing this forward. I didn't mean to disappear for a while. That documentation update looks good to me.

cymen avatar Nov 28 '22 18:11 cymen

No worries! As I said on the new PR, I'd be more than happy to continue on this feature on your existing PR here (but I couldn't work out a way to cherry-pick commits across forks, if that's even possible). Either way, I'm really excited that this is getting closer to a release, it'll be hugely helpful.

RiccardoMargiotta avatar Nov 28 '22 18:11 RiccardoMargiotta

@RiccardoMargiotta Sure, I'm happy to try -- I rebased my branch to master and cherry picked in your updates. I'm going to reread the documentation changes as the use case I wrote this for is slightly different so it might be useful to add another example there.

cymen avatar Nov 28 '22 19:11 cymen

Okay, I've updated the documentation for another use case -- it was a bit hard to condense the use case I wrote this originally for into a few sentences but hopefully it makes sense.

@justin808 Can you run the workflow on this PR?

cymen avatar Nov 28 '22 19:11 cymen

Really interesting to hear your use-case for this, and great to know it's been working well in production for you for so long - definitely a good sign!

Thanks for getting it all sorted on your original branch. I'm really looking forward to working with this on my app, glad I could help nudge it forward a little. What do you think, @justin808?

RiccardoMargiotta avatar Nov 28 '22 19:11 RiccardoMargiotta

  1. Should this PR have tests?
  2. What are the risks of this PR?
  3. We need an embedded example Rails app that demonstrates such new functionality, like https://github.com/shakacode/react_on_rails/tree/master/spec/dummy

justin808 avatar Jan 16 '23 23:01 justin808

@cymen It possibly got lost in the merge, but we're now missing line 9 of react_ujs/index.js.

var constructorFromRequireContextsWithGlobalFallback = require("./src/getConstructor/fromRequireContextsWithGlobalFallback")

Without this, we'll fail with:

ReferenceError: constructorFromRequireContextsWithGlobalFallback is not defined

RiccardoMargiotta avatar Jan 20 '23 14:01 RiccardoMargiotta

@justin808

  1. It should. But it doesn't appear like we have any tests for the react_ujs JS library part of the codebase at present, and quite honestly I wouldn't know where to begin.
  2. I think the risks are relatively low, given that it's an entirely new function based on an existing one. We won't have changed any existing use of the original function, people will have to opt-in to making use of this new feature.
  3. I'll try to create a sample app. Not something I've done before for a project like this, but I'll try to follow existing examples.

RiccardoMargiotta avatar Jan 20 '23 14:01 RiccardoMargiotta

@RiccardoMargiotta Oops, I think that did go missing in the merge. I've fixed that. I'll take a look at the merge conflict on the changelog.

cymen avatar Jan 22 '23 03:01 cymen

I took a look at adding testings -- I saw the test/dummy_webpacker3 directory and tried running that (after updating the ffi gem in order to get this running on a Mac M1). My thinking being that making a copy of this and using multiple contexts in the copy would be one way to go. However, it looks like the dummy Rails apps are not working.

For test/dummy_webpacker3 when running bundle exec rails server and attempting to access URL http://localhost:3000/pages/hi in a browser:

uninitialized constant PagesController::WebpackerHelpers
Rails.root: /Users/cymen/dev/react-rails/test/dummy_webpacker3

[Application Trace](http://localhost:3000/pages/hi#) | [Framework Trace](http://localhost:3000/pages/hi#) | [Full Trace](http://localhost:3000/pages/hi#)
[app/controllers/pages_controller.rb:2:in `<class:PagesController>'](http://localhost:3000/pages/hi#)
[app/controllers/pages_controller.rb:1:in `<top (required)>'](http://localhost:3000/pages/hi#)

For test/dummy_sprockets when running bundle exec rails server:

~/dev/react-rails/test/dummy_sprockets (master) % bundle exec rails server
Traceback (most recent call last):
	11: from bin/rails:4:in `<main>'
	10: from bin/rails:4:in `require'
	 9: from /Users/cymen/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/gems/railties-6.0.2.1/lib/rails/commands.rb:18:in `<top (required)>'
	 8: from /Users/cymen/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/gems/railties-6.0.2.1/lib/rails/command.rb:46:in `invoke'
	 7: from /Users/cymen/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/gems/railties-6.0.2.1/lib/rails/command/base.rb:69:in `perform'
	 6: from /Users/cymen/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/gems/thor-1.2.1/lib/thor.rb:392:in `dispatch'
	 5: from /Users/cymen/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/gems/thor-1.2.1/lib/thor/invocation.rb:127:in `invoke_command'
	 4: from /Users/cymen/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/gems/thor-1.2.1/lib/thor/command.rb:27:in `run'
	 3: from /Users/cymen/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/gems/railties-6.0.2.1/lib/rails/commands/server/server_command.rb:138:in `perform'
	 2: from /Users/cymen/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/gems/railties-6.0.2.1/lib/rails/commands/server/server_command.rb:138:in `tap'
	 1: from /Users/cymen/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/gems/railties-6.0.2.1/lib/rails/commands/server/server_command.rb:141:in `block in perform'
/Users/cymen/dev/react-rails/test/dummy_sprockets/config/application.rb:9:in `<top (required)>': uninitialized constant SprocketsHelpers (NameError)

I agree with @RiccardoMargiotta that is hard to know where to start. I also agree that there is very little risk because we're expanding the API by adding useContexts not changing the existing behavior by modifying the current useContext.

cymen avatar Jan 22 '23 04:01 cymen

@cymen and @RiccardoMargiotta, should we merge?

I can merge and put this into a 2.7 release.

justin808 avatar Feb 14 '23 07:02 justin808

@justin808 Sure, I think it's ready. Thanks!

cymen avatar Feb 14 '23 08:02 cymen

@justin808 I'd be good with merging! Like Cymen, I've forked out my own version of the JS package with this change, and have been using it in production for a while.

For my use case of route-based packs, it's been working great.

RiccardoMargiotta avatar Feb 14 '23 08:02 RiccardoMargiotta

Thanks @RiccardoMargiotta and @cymen!

justin808 avatar Feb 14 '23 08:02 justin808

Thanks @justin808! Appreciate your efforts on this project. 👍

RiccardoMargiotta avatar Feb 14 '23 09:02 RiccardoMargiotta