Skip to content

View Components: Recursively add scripts for subclass's parent component#7772

Merged
aduth merged 3 commits intomainfrom
aduth-view-component-script
Feb 7, 2023
Merged

View Components: Recursively add scripts for subclass's parent component#7772
aduth merged 3 commits intomainfrom
aduth-view-component-script

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Feb 6, 2023

🎫 Ticket

Extracted from #7761, related to LG-8771.

🛠 Summary of changes

Revises behavior of base script component to add scripts for the current class implementation, as well as any parent class scripts if the current component is subclassing another.

Essentially, given the following:

/components/parent_class_component.rb
/components/parent_class_component.ts
/components/child_class_component.rb
/components/child_class_component.ts

Before: When rendering ChildClassComponent, only child_class_component.ts would be added to the page
After: Both child_class_component.ts and parent_class_component.ts would be added to the page

Note that this seemed like an "obvious" behavior to me on Friday, though I'm of a less-strong opinion today, so happy to hear any dissenting opinion on whether it should work this way.

This should not impact any existing component, since while we do have some subclassing (e.g. DownloadButtonComponent < ButtonComponent), the parent classes in those cases do not have any associated scripts. This was discovered to be an issue while implementing #7761.

📜 Testing Plan

  • rspec spec/components/base_component_spec.rb

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

@scripts ||= _sidecar_files(['js', 'ts']).map { |file| File.basename(file, '.*') }
@scripts ||= begin
scripts = _sidecar_files(['js', 'ts']).map { |file| File.basename(file, '.*') }
scripts.concat superclass.scripts if superclass.respond_to?(:scripts)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think one of the things that was bugging me about the previous implementation was the specific BaseComponent being hardcoded, but switching this to superclass.respond_to?(:scripts) is way better IMO

@aduth aduth merged commit 8a04259 into main Feb 7, 2023
@aduth aduth deleted the aduth-view-component-script branch February 7, 2023 14:33
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.

2 participants