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

Railtie is optional dependency #63

Merged
merged 1 commit into from
Sep 20, 2022
Merged

Railtie is optional dependency #63

merged 1 commit into from
Sep 20, 2022

Conversation

kirs
Copy link
Contributor

@kirs kirs commented Sep 19, 2022

In a PR like https://github.com/Shopify/storefront-renderer/pull/15043 when you make app_profiler a dependency of a non-Rails app, it would still bring railties as a dependency. But that's not necessary and we could make Railtie an optional dependency just for Rails apps.

@dalehamel
Copy link
Member

I'm guessing this shouldn't cause problems for the majority of Ruby apps, as they are already rails apps and will already have railties as a dependency? And if they aren't a rails app, it should likewise be fine because the code simply won't be loaded?

@alexcoco
Copy link
Member

And if they aren't a rails app, it should likewise be fine because the code simply won't be loaded?

Having these dependencies creates extra load on the team to bump dependencies and understand how they are used within the app. If the dependencies are not needed I would prefer they were not dependencies at all.

@kirs
Copy link
Contributor Author

kirs commented Sep 20, 2022

they are already rails apps and will already have railties as a dependency?

Yes, exactly.

@kirs kirs merged commit c6f63b7 into main Sep 20, 2022
@kirs kirs deleted the railtie-optional branch September 20, 2022 13:53
This was referenced Sep 21, 2022
@shopify-shipit shopify-shipit bot temporarily deployed to production September 23, 2022 18:57 Inactive
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.

3 participants