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

Static components #20

Closed
wants to merge 1 commit into from
Closed

Static components #20

wants to merge 1 commit into from

Conversation

joeldrapper
Copy link
Collaborator

@joeldrapper joeldrapper commented Jul 14, 2022

Here's a rudimentary implementation of static components. Essentially, we render the component in the context of an object that returns interpolation strings for anything that's called on it.

I don't know if it's worth merging this or holding off and building something with syntax tree.

Closes #19

Comment on lines +7 to +9
def to_s
"#\{#{@value}}"
end
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Shouldn't use to_s here as it blocks us from recording to_s calls.

end

def method_missing(method_name, *args)
@value << "." << method_name.name
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The "." here should be a constant frozen string.

Choose a reason for hiding this comment

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

More to this, is string interpolation a bit faster than << ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's pretty negligible, but in theory interpolation should be slightly faster, yes.

Choose a reason for hiding this comment

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

Yeah, and no need to frozen string

Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines +3 to +4
def initialize(**kwargs)
kwargs.keys.each do
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
def initialize(**kwargs)
kwargs.keys.each do
def initialize(**assigns)
assigns.keys.each do

Comment on lines +10 to +12
(klass.instance_methods - StaticComponent.instance_methods).each do |m|
klass.define_method(m) { CallChainCollector.new(m) }
end
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's got to be a better way to do this.

end

def content(&)
raw '#{content(&)}'
Copy link
Collaborator Author

@joeldrapper joeldrapper Jul 14, 2022

Choose a reason for hiding this comment

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

raw needs implementing. It might be worth calling it something else to indicate that it shouldn't be used. Maybe just _raw or _raw_i_know_what_im_doing!!!!!!!!.

class StaticComponent < Component
def call
return super if is_a? Compiler
if respond_to? :compiled_template
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need a faster way to detect if a compiled template already exists. I think respond_to? is quite slow because it needs to look through the ancestry.

Choose a reason for hiding this comment

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

Maybe set instance variable once compiled_template is called ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, that's what I’m thinking. It’d be faster to check the instance variable.

end

def compile_template
compiler_class = Class.new(self.class).include(Compiler)

Choose a reason for hiding this comment

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

Is Compiler getting included at runtime ?

Copy link
Collaborator Author

@joeldrapper joeldrapper Jul 14, 2022

Choose a reason for hiding this comment

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

Yes. The only way to execute the template method against a new object is to subclass the component with that method, so the technique here is to subclass the component as an anonymous class and then include the Compiler functionality required to mock out the instance variables and methods.

I wish there was a way to instance_exec a bound method, e.g. method(:template) against another object like you can a block, but that doesn't seem possible.

Choose a reason for hiding this comment

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

Hmm ok, but didn't exactly get why Compiler can not be included at compile time. I'm thinking how much it can impact runtime processing for multiple instances otherwise 🤔

Copy link
Collaborator Author

@joeldrapper joeldrapper Jul 18, 2022

Choose a reason for hiding this comment

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

There isn't a compile time in Ruby, but we could definitely run the compilations on boot. At this point though, it's very unlikely I’ll merge this PR. I’m working on a new compiler that will be able to capture logic as well as markup strings.

This was totally worth building though as it proved it would work and showed how much faster compiled templates could be. Here's a benchmark I ran rendering 100k components.

FXGRtMuXkAATfvy

Choose a reason for hiding this comment

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

Ah yeah, I meant on boot time. Benchmark looks great!!

@joeldrapper
Copy link
Collaborator Author

I’m going to close this in favour of #19. It was worth prototyping something, but I don't think this is the right solution.

@joeldrapper joeldrapper deleted the static-components branch July 25, 2022 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Compiled components
3 participants