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

fix: remove type inferences from components #3195

Merged
merged 7 commits into from
Sep 3, 2024

Conversation

adrianthedev
Copy link
Collaborator

@adrianthedev adrianthedev commented Aug 27, 2024

Description

Fixes #3192

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

Copy link

codeclimate bot commented Aug 27, 2024

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

View more on Code Climate.

@rickychilcott
Copy link
Contributor

For my error (following https://docs.avohq.io/3.0/guides/rest-api-integration.html), I need to make this change to make it pass. I'm happy to make a separate PR, but it feels like it should be together.

FYI_Any does not include nil so you may need to do the same to fix #3192 for @xeron.

class Avo::Views::ResourceEditComponent
   ...
   prop :record, _Nilable(_Any)
   ...
end

@xeron
Copy link

xeron commented Aug 28, 2024

FYI_Any does not include nil so you may need to do the same to fix #3192 for @xeron.

I'm not setting current_user to nil, I'm passing a Struct, only name attribute is nil.

@@ -1,7 +1,7 @@
# frozen_string_literal: true

class Avo::SidebarProfileComponent < Avo::BaseComponent
prop :user, _Nilable(ActiveRecord::Base)
prop :user, _Any
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
prop :user, _Any
prop :user, _Nilable(_Any)

@rickychilcott
Copy link
Contributor

rickychilcott commented Aug 28, 2024

We discovered a second error today. So far the following would fix our two issues for us:

class Avo::PanelComponent < Avo::BaseComponent
  ...
  prop :name, _Nilable(_Union(_String, _Integer)) do |value|
    value || @args&.dig(:title)
  end
  ...
end

class Avo::Views::ResourceEditComponent < Avo::ResourceComponent
  ...
  prop :resource, _Nilable(_Any)
  ...
end

@rickychilcott
Copy link
Contributor

rickychilcott commented Aug 29, 2024

Lies... This is what I had to do (so far):

# in config/initializers/avo.rb

module OverrideAvoPanelComponent
  extend ActiveSupport::Concern

  included do
    prop :name, _Nilable(_Union(_String, _Integer)) do |value|
      value || @args&.dig(:title)
    end
  end
end

module OverrideAvoPanelNameComponent
  extend ActiveSupport::Concern

  included do
    prop :name, _Nilable(_Union(_String, _Integer))
  end
end

module OverrideAvoResourceEditComponent
  extend ActiveSupport::Concern

  included do
    prop :record, _Nilable(_Any)
  end
end

module OverrideAvoFieldBadgeViewerComponent
  extend ActiveSupport::Concern

  included do
    prop :value, _Union(_String, _Symbol)
  end
end

Rails.configuration.to_prepare do
  Avo::PanelComponent.include(OverrideAvoPanelComponent)
  Avo::PanelNameComponent.include(OverrideAvoPanelNameComponent)
  Avo::Fields::Common::BadgeViewerComponent.include(OverrideAvoFieldBadgeViewerComponent)
  Avo::Views::ResourceEditComponent.include(OverrideAvoResourceEditComponent)
end

@adrianthedev adrianthedev changed the title fix: remove ActiveRecord inferrence from component fix: remove type inferences from components Sep 3, 2024
@adrianthedev
Copy link
Collaborator Author

Thanks @rickychilcott for brainstorming this with us.
I don't quite get why cast the resource as any here prop :resource, _Nilable(_Any)?
I mean... we are expecting a resource to be given otherwise it will most likely crash.

@adrianthedev adrianthedev merged commit ac96632 into main Sep 3, 2024
22 checks passed
@adrianthedev adrianthedev deleted the fix/current-user-interface branch September 3, 2024 10:43
Copy link
Contributor

github-actions bot commented Sep 3, 2024

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

Please check the release guide for more information.

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.

current_user should not be limited to ActiveRecord::Base class
4 participants