Shopify / Shopify/identity_cache

Stop splitting code across coupled modules that are only included once

Open
#497 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

refactor
Dominant language
Ruby
Stars
2k
Forks
174
Avg merge
13m
Merged PRs (30d)
3

Description

Currently, IdentityCache::WithoutPrimaryIndex has the following single use internal module includes

    include IdentityCache::BelongsToCaching
    include IdentityCache::CacheKeyGeneration
    include IdentityCache::ConfigurationDSL
    include IdentityCache::QueryAPI
    include IdentityCache::CacheInvalidation
    include IdentityCache::ShouldUseCache
    include ParentModelExpiration

This is an anti-pattern that comes from not having proper separation of code into separate classes. To make things even worse, all this code is being included into model classes, so making any of the methods private doesn't actually prevent application code from calling them.

A better pattern to follow is the one taken by the classes in the IdentityCache::Cached namespace. They are built during initialization, which avoids adding extra object allocation on hot code paths, but still provides a place to store configuration state and methods that use that state. There may be more methods we can move into those classes and there may be opportunities to create new classes that are decoupled from the model classes.

The remaining methods on those single use internal module should be moved into IdentityCache::WithoutPrimaryIndex.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with IdentityCache::WithoutPrimaryIndex and review the listed included modules, then compare their responsibilities with the classes in the IdentityCache::Cached namespace. Done means the remaining methods from the single-use internal modules have been moved into IdentityCache::WithoutPrimaryIndex, with any additional class boundaries identified and validated by the project’s existing tests.

Written by the indexing model from the issue text.

Assessment

Tech stack
ruby
Domain
backend
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.