-
Notifications
You must be signed in to change notification settings - Fork 646
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
Shakapacker 7 compatibility (#1066) #1067
Shakapacker 7 compatibility (#1066) #1067
Conversation
if defined?(Shakapacker) | ||
Shakapacker::VERSION | ||
elsif defined?(Webpacker) | ||
Webpacker::VERSION |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you run your app locally with this fix? Aren’t there other places where the Webpacker constant is being referenced? Curious if this would fix the initial error but make way for another.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here for example:
if Webpacker.respond_to?(:dev_server) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi!
Yes I monkey patched it in my app, and it is working properly, running in production.
As I have stated in my PR description above:
- The issue is caused by this line:
require 'webpacker/version'
- The issue could be resolved by just removing this line. (
webpacker/version
would be present anyway, no need to require it again.) - To clarify once more, you will not get an error when referencing
Webpacker
in code, but you will get one when you try to requirewebpacker/version
.
This is happening because Shakapacker decided to slowly rename everything "webpacker" to "shakapacker" in their codebase. At the moment they are trying to provide backwards compatibility.
- This backwards compatibility is the reason why referencing
Webpacker
doesn't raise an error. When usingshakapacker
version7
. - But since they already renamed much of their source files, you can not directly require
webpacker/version
, because no such file exists. You would need to requireshakapacker/version
undershakapacker
version7
.- This is present in the project: https://github.com/shakacode/shakapacker/blob/v7.0.2/lib/shakapacker/version.rb
- This is not: https://github.com/shakacode/shakapacker/blob/v6.6.0/lib/webpacker/version.rb
If you do not like the idea of removing that require statement, then I can change my fix to the following:
def webpacker_version
if defined?(Shakapacker)
require 'shakapacker/version' # add new require statement for shakapacker version 7+
Shakapacker::VERSION
elsif defined?(Webpacker)
require 'webpacker/version' # keep the old require statement
Webpacker::VERSION
end
end
To clarify once more, wicked_pdf aside from this one line of code ( require 'webpacker/version'
) is shakapacker
version 7
compatible. If we remove or change this line in the above mentioned way, then we end up with full compatibility.
This full compatibility is the goal of my current PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@unixmonkey @AJFaraday I'm not sure how this was tested, but this is not working for us. We currently have version 2.6.3, and the change to 2.7.0 fails with an uninitialized constant Webpacker::VERSION error.
The same happens when I load the rails console and try to access the constant.
Note that nowhere in the webpacker code I see the require of the version file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sk- I would be happy to take a fix PR.
Oh I see, I didn't realiize the |
@unixmonkey are you ableto merge and release this? |
@sdrioux Can you confirm everything still works as expected if you install this branch into your project like: gem 'wicked_pdf', github: 'Gregurevic/wicked_pdf', ref: '995126b441db21eb39ff964fc3fee9e2babe08ed' |
@unixmonkey I've been looking at this same issue and I can confirm that by using that line in my gemfile it worked as expected. |
Thank you for confirming this @AJFaraday . |
Hi!
Issue
I'm using
wicked_pdf
in a project where assets are being delivered withshakapacker
version7
.I ran into an issue where the following error message was raised when I tried to use pack asset helpers:
I've seen that this issue has already been reported here: #1066
Cause
I've found out that this line was raising the error:
lib/wicked_pdf/wicked_pdf_helper/assets.rb :237
Shakapacker started renaming their entire project from
webpacker
toshakapacker
.See the changelog for details: https://github.com/shakacode/shakapacker/blob/master/CHANGELOG.md#changed
Fix
A possible fix could have been to just remove the line raising the error. I believe that when
defined?(Webpacker)
is true, we do not need torequire 'webpacker/version'
. It will already be defined.I figured tho that a more long lasting solution would be to check if either of Shakapcker or Webpacker is defined. In case shakapacker's backward compatibility expires.
Tests
I did not include any tests in my PR. The current test suite doesn't have the ability to test assets delivered by shakapacker. I believe that the CI test matrix would need to be expanded and the gemfiles would need to be reworked to do so. I've found that to be a lot larger of a fix than what I originally intended to do.