Support singleton webpacker (3 and above) #777
Conversation
| private | ||
|
|
||
| def webpack_configuration | ||
| Webpacker.respond_to?(:config) ? Webpacker.config : webpack_configuration |
There was a problem hiding this comment.
should be
Webpacker.respond_to?(:config) ? Webpacker.config : Webpacker::ConfigurationThere was a problem hiding this comment.
Aaw whoops. Wondered why my test didn't pass XD Thanks
There was a problem hiding this comment.
I've still got issues around Webpacker::Manifest.load, seems to be how I detect respond_to? may be wrong for that one.
| Webpacker::Configuration.source_path | ||
| .join(Webpacker::Configuration.entry_path) | ||
| webpack_configuration.source_path | ||
| .join(webpack_configuration.entry_path) |
There was a problem hiding this comment.
The new singleton implementation of Webpacker::Configuration no longer has entry_path. Instead, Webpacker::Configuration.source_path.join(Webpacker::Configuration.entry_path) becomes Webpacker.config.source_entry_path
There was a problem hiding this comment.
I have suggested these changes in issue #778 .Please have a look into it. I was trying to install react with rails and failed to run react installer command. I look into gem and found that method calling is not proper.
I fix this with minor changes in lib/generators/react/install_generator.rb file.
source_path is a instance method and was being called by class directly. Also entry_path was changed to source_entry_path in webpacker config. I have suggested these changes in #778
The main purpose for issue was #778 was to elevate exact issue and its solution along with it, as there was no fix until that time.
There was a problem hiding this comment.
Using webpacker 3.0.1 Webpacker.config.source_entry_path worked for me 👍
There was a problem hiding this comment.
It looks good, only a minor change suggested that webpack_configuration.entry_path should be changed to webpack_configuration.source_entry_path, as entry_path is renamed as source_entry_path in webpacker config file. File location for this file is lib/generators/react/install_generator.rb under method name javascript_dir.
|
I took on the code from #778 , thank you for that one! Makes it a little easier to update. Currently working on trying to make the new Will take me some time to figure out all the subtleties of this before I'm done. Very welcome for someone to help with this! Edit: Is there a nice way of changing the config files in |
There was a problem hiding this comment.
In file lib/react/server_rendering/webpacker_manifest_container.rb, I am looking at a method name lookup_path at line no. 24, but I don't think it is defined anywhere in the both react-rails & webpacker gem. This will through an error once find_asset(logical_path) method is being called.
Please look into it! Are you guys loading any file with such variable?
Apart for this, hello @BookOfGreg, what kind of changes you wanna in test/dummy config files. Does it contains any Json data? Please share, hopefully I may change that!
|
Webpacker 1 and 2 have |
|
I'm not sure if this is related, but the component generator is putting my components in app/javascript/packs/components. To get this to work, I had to change the require context from 'components' to './components' And also in my es6 components I have to require('../components/NameOfComponent') |
test/server_rendered_html_test.rb
Outdated
| var NewList = function() { return <span>"New List"</span> } | ||
| HEREDOC | ||
| new_file_contents = <<-HEREDOC | ||
| var NewList = function() { return <span>"New List"</span> } |
There was a problem hiding this comment.
Seems like you might've messed up the indentation here
There was a problem hiding this comment.
The <<- syntax allows the closing HEREDOC to be indented but it won't strip space from the content so this works just fine.
| end | ||
|
|
||
| def config | ||
| !!defined?(Webpacker::Configuration) ? Webpacker::Configuration : Webpacker.config |
There was a problem hiding this comment.
I think this needs to be
def config
Webpacker.respond_to?(:config) ? Webpacker.config : Webpacker::Configuration
endBecause even in Webpacker 3, the Webpacker::Configuration constant is defined.
|
Just ran into issue #778, and this fixed it for me. 👍 |
|
I'll admit that I'm at a bit of a dead end. On #779 I've got all of Travis passing except Rails 5 + Webpacker, but on that branch Webpack builds to public/packs but Webpacker looks in packs/packs instead. That is the case even if I peg Webpack at 1.15.x and Webpacker at ~> 1.0. We should really think about the maintainability of the Webpacker support as the manifest paths between versions seems really flakey and i'm not keen on the pattern of .responds_to? and defined? everywhere. |
… message changes between versions
|
Master finally passes and this does mean Webpacker 3 support (And 1.1, 1.2, 2.0) on this branch. |
| @@ -1,4 +1,5 @@ | |||
| require "open-uri" | |||
| # require 'pry' | |||
|
Wow, this is really impressive! Thanks for all your work in digging in on this. It's nice to see that Webpacker has provided some APIs to support this kind of thing, too. I see the green check, and the tests look good. So, what's still to go on this one? |
|
👍 |
|
This could go in to master it's current state as I believe it to be an improvement but frankly this could use some TLC and refactoring. I'm certain all my I found some odd behaviour such as webpack 3 seems to read the wrong node_modules folder when loading in All these tweaks could be in future patches to this though. Edit: Since I've modified the webpacker 1.1, 1.2 and 2 support, it would be good to see if it still works without bugs on an existing project for confirmation. |
|
You beast @BookOfGreg 👍 ❗️ |
|
I think we should probably merge this if we can be confident that it works. All existing tests pass. I can carry on with a tidy up beyond this. Edit: apparently linting broke the tests. How odd. Will keep looking into it. |
|
Tests fail on specific seeds: |
|
@rmosolgo If you like, |
and I'll keep PR'ing rubocops, test fixes and refactorings as patch-level changes after this. Be good to start getting some bug reports from folks so I know if this accidentally impacts anyone.
|
That sounds great, let me go ahead with a release! |
|
I'm still having the #778 issue and was wondering if this latest merge had been release to fix it or if there is a workaround on my end that I can do before a fix is released, thanks for all the work you do! |
|
@gaiapunk It's not been released as a gem yet but it's possible to use the master branch directly with: |
Fixes #776
Fixes #775
Fixes #778
Fixes #780
Fixes #781
Fixes #782
Once again I could probably use some assistance with the appraisal gem but other than that should be OK.
Edit: This turned into a BIG refactor of the tests to split out support and I'll be honest my code isn't the tidiest. Still plenty of refactoring to go as I'm not sure I'm thrilled about how I've switched method definitions of Webpacker version numbers.