Skip to content

Conversation

binarygit
Copy link
Contributor

@binarygit binarygit commented Aug 7, 2024

This PR replaces the initialize method on components to use the prop method from the literal gem which provides a way to specify type.

Description

Fixes #2974
Fixes #2893

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

Screenshots & recording

Manual review steps

  1. Step 1
  2. Step 2

Manual reviewer: please leave a comment with output from the test if that's the case.

Copy link

qlty-cloud-legacy bot commented Aug 7, 2024

Code Climate has analyzed commit 6dc34c2 and detected 0 issues on this pull request.

View more on Code Climate.

@binarygit binarygit force-pushed the literal-gem branch 2 times, most recently from b5f6f59 to c94369d Compare August 8, 2024 16:36
@binarygit binarygit force-pushed the literal-gem branch 2 times, most recently from 0f8d440 to 35e7208 Compare August 10, 2024 05:44
@binarygit binarygit marked this pull request as ready for review August 12, 2024 06:10
Copy link
Contributor

@Paul-Bob Paul-Bob left a comment

Choose a reason for hiding this comment

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

I left some questions.

I noticed that we have many props with reader: :public where the reader is not necessarily needed as public. I know this PR respects the previous attr_reader and converts it to reader: :public but using the instance variable is faster than accessing through the reader.

Whenever there is a case where the reader is not necessary since the attribute is used only on the object's scope lets remove the reader and use the instance variable in favor.

@binarygit binarygit requested a review from Paul-Bob August 16, 2024 05:57
Copy link
Contributor

@Paul-Bob Paul-Bob left a comment

Choose a reason for hiding this comment

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

I left some questions and some tweaks comments, overall is looking great, thank you for changing the readers to ivars!

Copy link
Contributor

@Paul-Bob Paul-Bob left a comment

Choose a reason for hiding this comment

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

Thank you for the effort on this one @binarygit!

@Paul-Bob Paul-Bob merged commit ff35408 into main Aug 19, 2024
22 checks passed
@Paul-Bob Paul-Bob deleted the literal-gem branch August 19, 2024 10:51
Copy link
Contributor

This PR has been merged into main. The functionality will be available in the next release.

Please check the release guide for more information.

@Paul-Bob Paul-Bob added the Chore label Aug 19, 2024
@xeron
Copy link

xeron commented Aug 23, 2024

@binarygit @Paul-Bob this broke my custom code for setting current user based on HTTP basic auth:

/app/avo/concerns/avo_current_user.rb:

module AvoCurrentUser
  extend ActiveSupport::Concern

  User = Struct.new(:name)

  included do
    before_action :set_paper_trail_whodunnit
  end

  protected

    def user_for_paper_trail
      current_user.name
    end

    def current_user
      user, _pass = ActionController::HttpAuthentication::Basic.user_name_and_password(request)
      User.new(user)
    end
end

/config/initializers/avo.rb:

Rails.configuration.to_prepare do
  Avo::ApplicationController.include AvoCurrentUser
end
13:41:36 web.1  | ActionView::Template::Error (Expected `#<struct AvoCurrentUser::User name=nil>` to be of type: `_Nilable(ActiveRecord::Base)`.):
13:41:36 web.1  |
13:41:36 web.1  | Causes:
13:41:36 web.1  | Literal::TypeError (Expected `#<struct AvoCurrentUser::User name=nil>` to be of type: `_Nilable(ActiveRecord::Base)`.)
13:41:36 web.1  |     32:         <%= render partial: "avo/partials/navbar" %>
13:41:36 web.1  |     33:         <div data-sidebar-target="mainArea" class="content-area flex-1 flex pt-16 relative <%= "sidebar-open" if @sidebar_open %>">
13:41:36 web.1  |     34:           <div class="hidden lg:flex">
13:41:36 web.1  |     35:             <%= render Avo::SidebarComponent.new sidebar_open: @sidebar_open %>
13:41:36 web.1  |     36:           </div>
13:41:36 web.1  |     37:           <div class="flex lg:hidden">
13:41:36 web.1  |     38:             <%= render Avo::SidebarComponent.new sidebar_open: false, for_mobile: true %>
13:41:36 web.1  |
13:41:36 web.1  | (eval):7:in `initialize'

@resource = resource
end
prop :field, Avo::Fields::BaseField
prop :file, ActiveStorage::Attachment

Choose a reason for hiding this comment

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

This breaks eager-loading on rails apps that dont have active_storage/engine required

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! We missed it before.
Fixed by #3215

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reopen #2893 (Apply initial Literal gem changes)
5 participants