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

Only require parts of Rails actually needed #60

Merged
merged 1 commit into from
Nov 9, 2023

Conversation

bdewater
Copy link
Contributor

@bdewater bdewater commented Apr 30, 2023

In our app we don't need Action Cable/Text/Mailbox etc and we don't want to pull it in for this gem.

It would be nice to even make Active Record optional and have a pattern to only offer functionality for Rails components used in the consuming application.

Relates to #47 but is more of a quick fix - I think it's possible to depends on Railties alone but this solves my immediate problem.

@bdewater bdewater requested a review from a team as a code owner April 30, 2023 20:58
@bdewater bdewater force-pushed the less-deps branch 3 times, most recently from 4d8bcd3 to 87a7453 Compare May 1, 2023 23:05
@vinistock
Copy link
Member

Thanks for the contribution!

Instead of completely removing all the parts that aren't used now, can we restrict our runtime dependencies and then add the other parts to the Gemfile?

We may want to add functionality related to jobs for example, in which case we'd need to reintroduce the parts again.

If we can restrict the dependencies of the gem, without losing the other parts for development, that'd be great.

@bdewater
Copy link
Contributor Author

@vinistock sorry for the late response, I was on pat leave :). I'll rebase this later this week.

Having to re-add parts of Rails explicitly to me felt like a good thing, so devs do not unintentionally add functionality relying on parts of the framework not declared as dependencies.

@rafaelfranca
Copy link
Member

I agree wit Bart, we should not add Rails in the Gemfile.

@tiagotex
Copy link

tiagotex commented Nov 7, 2023

I'm in the same boat as Bart. Our app only loads the parts of Rails we use, and using ruby-lsp-rails now forces us to load it all.

I'm available to help make any changes to get this merged :)

Thanks for the awesome gem! 💚

@bdewater
Copy link
Contributor Author

bdewater commented Nov 8, 2023

Rebased! Seems the CLA check is broken.

@st0012
Copy link
Member

st0012 commented Nov 9, 2023

@bdewater some dependencies seem downgraded in this PR and CI is failing due to dependency issues. Can you resolve it and push again?

@bdewater
Copy link
Contributor Author

bdewater commented Nov 9, 2023

@st0012 should be good now :)

In our app we don't need Action Cable/Text/Mailbox etc and we don't want to pull it in for this gem.
Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR 👍

@st0012 st0012 merged commit 2601c35 into Shopify:main Nov 9, 2023
14 checks passed
@bdewater bdewater deleted the less-deps branch November 9, 2023 21:11
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.

None yet

5 participants