react-rails
react-rails copied to clipboard
Add support for multiple `require.context` with addition of `useContexts`
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.
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. 😃
@cymen, thanks for your contribution. Sorry for the delays to get this merged and shipped. Looks useful!
Please:
- rebase on master
- add unit tests
- add a CHANGELOG.md entry
- improve the clarity of the docs
Thanks!
@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:
- rebase on master
- add unit tests
- add a CHANGELOG.md entry
- improve the clarity of the docs
Thanks!
@RiccardoMargiotta do you have time to push a new PR that includes your suggestions?
@justin808 I've created https://github.com/reactjs/react-rails/pull/1221. Could you run the workflow please?
@RiccardoMargiotta Thanks for pushing this forward. I didn't mean to disappear for a while. That documentation update looks good to me.
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 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.
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?
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?
- Should this PR have tests?
- What are the risks of this PR?
- We need an embedded example Rails app that demonstrates such new functionality, like https://github.com/shakacode/react_on_rails/tree/master/spec/dummy
@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
@justin808
- It should. But it doesn't appear like we have any tests for the
react_ujsJS library part of the codebase at present, and quite honestly I wouldn't know where to begin. - 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.
- 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 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.
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 and @RiccardoMargiotta, should we merge?
I can merge and put this into a 2.7 release.
@justin808 Sure, I think it's ready. Thanks!
@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.
Thanks @RiccardoMargiotta and @cymen!
Thanks @justin808! Appreciate your efforts on this project. 👍