Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Shakapacker 7 compatibility (#1066) #1067

Merged
merged 1 commit into from Aug 17, 2023
Merged

Shakapacker 7 compatibility (#1066) #1067

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 5 additions & 6 deletions lib/wicked_pdf/wicked_pdf_helper/assets.rb
View file
Open in desktop
Expand Up @@ -231,12 +231,11 @@ def running_in_development?
end

def webpacker_version
return unless defined?(Webpacker)

# If webpacker is used, need to check for version
require 'webpacker/version'

Webpacker::VERSION
if defined?(Shakapacker)
Shakapacker::VERSION
elsif defined?(Webpacker)
Webpacker::VERSION
Copy link

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.

Copy link

Choose a reason for hiding this comment

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

Here for example:

wicked_pdf/lib/wicked_pdf/wicked_pdf_helper/assets.rb

Line 226 in 4abf255

if Webpacker.respond_to?(:dev_server)

Copy link
Contributor Author

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 require webpacker/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.

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.

Copy link

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.

Copy link
Collaborator

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.

end
end
end
end
Expand Down