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.

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

Merged
merged 1 commit into from Aug 17, 2023

Conversation

Gregurevic
Copy link
Contributor

Hi!

Issue

I'm using wicked_pdf in a project where assets are being delivered with shakapacker version 7 .
I ran into an issue where the following error message was raised when I tried to use pack asset helpers:

cannot load such file -- webpacker/version (LoadError)

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

require 'webpacker/version'

Shakapacker started renaming their entire project from webpacker to shakapacker .
See the changelog for details: https://github.com/shakacode/shakapacker/blob/master/CHANGELOG.md#changed

Rename Webpacker to Shakapacker in the entire project including config files, binstubs, environment variables, etc. with a high degree of backward compatibility.

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 to require '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.

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:

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.

@Gregurevic Gregurevic requested a review from sdrioux July 31, 2023 09:04
@sdrioux
Copy link

sdrioux commented Aug 15, 2023

Oh I see, I didn't realiize the Webpacker constant was still supported. Great! How do we get this released?

@sdrioux
Copy link

sdrioux commented Aug 15, 2023

@unixmonkey are you ableto merge and release this?

@unixmonkey
Copy link
Collaborator

@sdrioux Can you confirm everything still works as expected if you install this branch into your project like:
Gemfile

gem 'wicked_pdf', github: 'Gregurevic/wicked_pdf', ref: '995126b441db21eb39ff964fc3fee9e2babe08ed'

@AJFaraday
Copy link

@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.

@unixmonkey
Copy link
Collaborator

Thank you for confirming this @AJFaraday .

@unixmonkey unixmonkey merged commit 3b99205 into mileszs:master Aug 17, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants