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

Initial batch of swapping Active Support extensions for in-house alternatives (WIP) #881

Merged
merged 30 commits into from
Jul 14, 2024

Conversation

jaredcwhite
Copy link
Member

@jaredcwhite jaredcwhite commented Apr 13, 2024

Congruent with our stated goals for Bridgetown 2.0 here: https://www.bridgetownrb.com/future/road-to-bridgetown-2.0-escaping-burnout/

This PR offers an initial batch of in-house Ruby extensions in lieu of using Active Support dependencies. It also sets up a paradigm of using Zeitwerk and modules to manage core object inclusions. Update: see below for thoughts about mostly relying on refinements vs. standard monkey-patches.

  • .questionable lets us continue using the nicety of Bridgetown.env.development?
  • .starts_with? and ends_with? as alias of start_with? and end_with?
  • .within? as a souped-up version of in? — companions of Module/Classes, Sets, and Hashes are nicer because they use the < operator. Makes sense that {easy_as: 123} would be .within?({indeed: "it's true", easy_as: 123}). Arrays/Arrays use .difference under the hood, so that [1,2] would be within [5,4,3,2,1]
  • .nested_within? lets you see if Foo::Bar is nested within Foo, and you can also access nested parent modules/classes with .nested_parents
  • Also added: translation safety
  • A simplistic deep_dup for Hashes/Arrays
  • hash_with_dot_access has been rewritten entirely to remove AS dependency, and is now faster due to defining accessor methods on first method_missing calls.
  • Update: moved the base Model class (aka the raw data underlying a Resource) completely away from Active Model. Callbacks removed too. A lot of concepts just never gelled anyway (like adding validation to individual model classes?!) and were intended for a CMS-like use case I'm moving away from anyway (a future "Bridgetown CMS" would likely be headless where the UI and the data models live in separate services).
  • Callbacks for Builders has been refactored, and it's a bit simpler. I'm not entirely sure we even need them upon reflection, but it seems potentially useful for extending gem-based builders…
  • Update: We're now using a subclass of Dry::Inflector as the basis for our native Bridgetown inflection support. updated docs here

Note that not all extensions will necessarily make the cut as we go through this process, but these all feel like worthwhile endeavors to me.

Feedback welcome!

…rnatives

- new `within?` method offers some unique semantics
@jaredcwhite jaredcwhite added this to the 2.0 milestone Apr 13, 2024
Copy link

render bot commented Apr 13, 2024

Copy link

render bot commented Apr 13, 2024

@jaredcwhite jaredcwhite changed the title Initial batch of swapping Active Support extensions for in-house alternatives Initial batch of swapping Active Support extensions for in-house alternatives (WIP) Apr 13, 2024
@ayushn21
Copy link
Member

Is it worth separating our extensions from bridgetown-core into another gem still contained within our monorepo?

@jaredcwhite
Copy link
Member Author

jaredcwhite commented Apr 13, 2024

@ayushn21 Ooo, I love that idea! I can immediately think of a few other existing core utilities we could extract out as well. Could then be a nice little standalone gem.

Any ideas on what to call it?

@ayushn21
Copy link
Member

It's not the most creative name, but maybe bridgetown-ruby? That signifies we're creating a dialect under the Ruby stdlib for use within Bridgetown.

@jaredcwhite
Copy link
Member Author

@ayushn21 Ah, hmm well that would keep the scope pretty small (and not appropriate for the utilities I was thinking about). Let me chew on that…

- Remove unneeded Active Support extensions once thought to be necessary for Thor actions
@jaredcwhite
Copy link
Member Author

@ayushn21 what do you think about bridgetown-lib?

@brandondrew
Copy link

bridgetown-support?
bridgetown-extensions?

@ayushn21
Copy link
Member

bridgetown-foundation?

@brandondrew
Copy link

abutment?

(An abutment is the part of a bridge that provides the support necessary to distribute the weight and stresses from the bridge to the ground, and typically anchors the bridge at either end.)

@jaredcwhite
Copy link
Member Author

abutment?

@brandondrew I like the direction you're headed in…too bad that word immediately gets me thinking about juvenile humor… 😏😂

@brandondrew
Copy link

@brandondrew I like the direction you're headed in…too bad that word immediately gets me thinking about juvenile humor… 😏😂

yeah, I should have thought of that...

@pablojimeno
Copy link

What about Bridgetown Bridge? I think the repetition sounds great, but also the concept and the meta reference are interesting.

@brandondrew
Copy link

bridgetown-pillar or bridgetown-pillars

@jaredcwhite
Copy link
Member Author

jaredcwhite commented Apr 14, 2024

@brandondrew Or could go with bridgetown-piers — I did like 15 seconds of research and it seems the columns that hold up a bridge above water are called piers. 🤷🏻‍♂️

@ayushn21
Copy link
Member

bridgetown-piers reminds of that loathsome human Piers Morgan 😂.

I'm quite partial to bridgetown-foundation because I came up with it (obviously).

But it's also a nice parallel to the iOS/Mac platform. There you have the base language (Swift or Obj-C) and then a framework called "Foundation" which sits on top of that containing a bunch of Apple system APIs. On top of that you have the framework for the platform you're developing on (UIKit, AppKit, whatever else...).

@jaredcwhite
Copy link
Member Author

@ayushn21 Let's go with that then. Sounds good to me!

@jaredcwhite jaredcwhite added process Improve the development process for the repo dependencies Pull requests that update a dependency file labels Apr 16, 2024
@jaredcwhite
Copy link
Member Author

jaredcwhite commented Apr 19, 2024

Bit of a direction switch with the latest commit…I have been an unabashed fan of Ruby refinements for the longest time, but a while back I was sort of talked out of the idea due to the performance hit of using a refined method.

Well I'm coming back around to the idea of it being a good thing. I did some benchmarking on a String using a refined method vs a standard monkey-patched method, and standard monkey-patching was about 1.25x the performance. So the question starts to become, how often does this a refined method get called during a span of time (a build process or an HTTP request, essentially)? 1 time? 5 times? 100 times? Unless it's being called literally thousands of times (I only started to see major "real time" difference in the millions), I think we're splitting hairs. We can identify certain code paths where cumulative performance is absolutely critical (so I might back out of a few changes in that recent commit), but otherwise I think it's worth it.

"indent me!".indent(2) # NoMethodError

using Bridgetown::Refinements

"indent me!".indent(2) # "  indent me!"

is just so cool. Of course you could also just reach for individual refinements, which is as close to "imports" as you'll get in Ruby:

"indent me!".indent(2) # NoMethodError

using Bridgetown::Foundation::RefineExts::String

"indent me!".indent(2) # "  indent me!"

I'm still sussing out what we might want as core extensions globally vs. refinements. SomeClass.descendants seems like a no-brainer as a core ext—frankly it should be in Ruby proper. And starts_with? as an alias to start_with? also seems like a no-brainer. Other stuff gets murky real fast…and I'd much feel more cautious/anxious patching any Ruby core classes if I knew it was a global change which we'd have to support virtually forever and that might conflict with both third-party gems and future Rubies themselves. (Hence the very real downsides of an ActiveSupport in the first place!)

Thoughts? Questions? Ideas?

@jaredcwhite
Copy link
Member Author

OK, this PR is fairly well baked at this point…I'll leave it open for a while for anything else which feels like low-hanging fruit, but generally it seems in good shape to pull into the v2.0 release.

@jclusso
Copy link
Contributor

jclusso commented Apr 26, 2024

I checked this out to see if it broke anything. Trying to run a build for us results in the following error. I looked through this PR, but wasn't able to hunt down what cause this. I'm not sure if this is something with Bridgetown::Component or something we're using in ActiveSupport that was removed.

[Bridgetown] Stopping auxiliary processes...                                                                        
[Bridgetown]   Exception raised: NameError                                                                          
[Bridgetown] uninitialized constant ActionView::OutputBuffer                           
[Bridgetown]                  1: /Users/jarrett/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/actionview-7.1.3.2/lib/action_view/helpers/capture_helper.rb:49:in `capture'                                                             
[Bridgetown]                  2: /Users/jarrett/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/bundler/gems/bridgetown-e172990a669c/bridgetown-core/lib/bridgetown-core/component.rb:167:in `render' 

@jaredcwhite
Copy link
Member Author

@jclusso Hmm…well Action View was never something we pulled into core, but that gets me wondering if you are also using ViewComponent?

@jclusso
Copy link
Contributor

jclusso commented Apr 26, 2024

@jclusso Hmm…well Action View was never something we pulled into core, but that gets me wondering if you are also using ViewComponent?

We're just using Bridgetown::Component. We do pull in ActionView.

class SiteComponent < Bridgetown::Component
  include ActionView::Helpers::TagHelper
end

@jclusso
Copy link
Contributor

jclusso commented Apr 26, 2024

If I follow the stack trace above it leads me to:

which inherits from ActiveSupport::SafeBuffer.

class OutputBuffer < ActiveSupport::SafeBuffer

I'm not sure how any of this works really.

@jclusso
Copy link
Contributor

jclusso commented Jul 1, 2024

If I follow the stack trace above it leads me to:

which inherits from ActiveSupport::SafeBuffer.

class OutputBuffer < ActiveSupport::SafeBuffer

I'm not sure how any of this works really.

Were you ever able to hunt anything down related to why this breaks?

@jaredcwhite
Copy link
Member Author

jaredcwhite commented Jul 1, 2024

@jclusso I don't have a clear idea, but I'm wondering if you'd need to add a require for action_view/buffers — it sounds like the ActionView::OutputBuffer class isn't getting loaded.

@jaredcwhite
Copy link
Member Author

Going to add a separate issue for some documentation on this, but otherwise…LGTM!

@jaredcwhite jaredcwhite merged commit 98b6a7b into main Jul 14, 2024
3 checks passed
@jaredcwhite jaredcwhite deleted the swap-active-support-core-exts branch July 14, 2024 14:25
@jclusso
Copy link
Contributor

jclusso commented Jul 15, 2024

@jclusso I don't have a clear idea, but I'm wondering if you'd need to add a require for action_view/buffers — it sounds like the ActionView::OutputBuffer class isn't getting loaded.

I can confirm this fixes it. Not sure where this should go, but documentation should be added for people upgrading to know if they rely on Active Support that they need to add them gem and require it.

I currently have required active_support/all and action_view/buffers in the site_builder.rb, but not sure if that's the best place?

@jaredcwhite
Copy link
Member Author

@jclusso OK fabulous. I'll make a note to include this in an upgrade guide.

@jaredcwhite
Copy link
Member Author

site_builder.rb is fine, but ideally I'd say it should go in config/initializers.rb.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file process Improve the development process for the repo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants