kpumuk / meta-tags

Search Engine Optimization (SEO) for Ruby on Rails applications.
MIT License
2.73k stars 275 forks source link

Separate RBS files to _internal directory to avoid exposing RBS #237

Closed pocke closed 2 years ago

pocke commented 2 years ago

This PR moves RBS declarations to _internal directory to avoid exposing internal RBS.

Problem

The method definitions for Hash and Object in the RBSs are duplicated with ActiveSupport's RBS. For example: https://github.com/ruby/gem_rbs_collection/blob/6320a194c85afbf4ff332c3eb75fcae2a6518c92/gems/activesupport/6.0/activesupport-generated.rbs#L3298

RBS doesn't allow duplicated method definitions, so we cannot use meta-tags gem and ActiveSupport RBS at the same time. For example:

$ cat Gemfile
source "https://rubygems.org"

gem "activesupport"
gem 'meta-tags'
gem 'rbs'

$ bundle install
# Install meta-tags RBS from sig/ directory and ActiveSupport RBS from ruby/gem_rbs_collection repo
$ bundle exec rbs collection init && bundle exec rbs collection install

$ rbs validate
Validating class/module definition: `::Array`...
bundler: failed to load command: rbs (/path/to/.rbenv/versions/trunk/lib/ruby/gems/3.2.0/bin/rbs)
/path/to/lib/rbs/definition_builder/method_builder.rb:36:in `block in validate!': .gem_rbs_collection/activesupport/6.0/activesupport-generated.rbs:4588:2...4588:27: ::Object#blank? has duplicated definitions in /path/to/meta-tags-2.16.0/sig/lib/_rails.rbs:8:2...8:24 (RBS::DuplicatedMethodDefinitionError)
        from /path/to/lib/rbs/definition_builder/method_builder.rb:34:in `each_value'
        from /path/to/lib/rbs/definition_builder/method_builder.rb:34:in `validate!'
        from /path/to/lib/rbs/definition_builder/method_builder.rb:139:in `build_instance'
        from /path/to/lib/rbs/definition_builder.rb:148:in `block (2 levels) in build_instance'
(snip)

Solution

Move the conflicted RBSs to _internal directory.

RBS ignores directories that start with _ if the directory is loaded as a library. So when you want to make a polyfill, you can use _internal directory. It is loaded when we develop meta-tags gem itself, but it is not loaded when we use meta-tags gem from other application/library.

I also moved the interfaces under MetaTags namespace. The interfaces are necessary for exposed RBSs, so I didn't move them to _internal directory. They are defined at the top level, so I moved them under the namespace to avoid conflicting other RBS.

Alternative Solution

We can also use the ActiveSupport's RBS from ruby/gem_rbs_collection repository instead of the polyfill. But I didn't recommend this way for now.

Because currently Steep supports rbs collection feature partially. rbs collection is the library management system for RBS, like Bundler. You can install dependencies' RBSs with the command out of the box. RBS v1 provides it as an experimental feature and it's GA on RBS v2. The latest Steep is not compatible with RBS v2. So to use rbs collection, we need to use the experimental one, lack of some features, or use the master branch of Steep.

I think it is premature to use stably, so I recommend using the polyfill.


A lot of thanks to writing RBSs!

kpumuk commented 2 years ago

Thank you for the detailed investigation!

natematykiewicz commented 2 years ago

I don't think the type signature for ::Object#presence is right.

class ::Object
  def presence: () -> String?
end

The definition of presence is:

class Object
  def presence
    self if present?
  end
end

https://github.com/rails/rails/blob/f95c0b7e96eb36bc3efc0c5beffbb9e84ea664e4/activesupport/lib/active_support/core_ext/object/blank.rb#L45-L48

So, presence returns the receiver or nil, not String or nil.

Note: I'm very new to RBS, so I might be wrong here. I just looked at this PR, because Dependabot told me about meta-tags 2.17.0.

kpumuk commented 2 years ago

Thanks @natematykiewicz

natematykiewicz commented 2 years ago

No problem!