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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make haml/rbs dependencies optional #1406

Merged
merged 3 commits into from
Nov 21, 2023

Conversation

mikesea
Copy link
Contributor

@mikesea mikesea commented Nov 17, 2023

馃憢 Hey, thanks for this plugin.

Right now this plugin requires you to install the dependencies for formatting haml and rbs files, even if your codebase isn't using those types of files. This PR modifies src/server.rb so these formatting dependencies are only required "on demand" - e.g., when the server is actually processing a haml or rbs file only required optionally.

This change will mean we can drop haml as a dependency in our codebase, since it's only being imported so that we can format ruby files with this plugin.

@kddnewton
Copy link
Member

Hi @mikesea! Thanks for the contribution. I can totally see where you're coming from.

I don't want to call require on every file being formatted. Could we instead only require at the top of the file but rescue LoadError for the ones that should be optional? That way they still wouldn't be "required" but it wouldn't slow down individual requests once the server is booted.

@kddnewton
Copy link
Member

Also you should rebase, main was broken, but it's been fixed now.

Copy link
Member

@kddnewton kddnewton left a comment

Choose a reason for hiding this comment

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

Just marking this as "request changes"

@mikesea
Copy link
Contributor Author

mikesea commented Nov 20, 2023

@kddnewton makes sense, done!

@mikesea mikesea changed the title Require haml/rbs dependencies on demand Make haml/rbs dependencies optional Nov 20, 2023
@kddnewton
Copy link
Member

@mikesea looks like you just need to run prettier on server.rb now

@mikesea
Copy link
Contributor Author

mikesea commented Nov 21, 2023

looks like you just need to run prettier on server.rb now

馃う

@kddnewton sorry about that, done!

@kddnewton kddnewton merged commit 52418ab into prettier:main Nov 21, 2023
12 checks passed
@langalex
Copy link

woudln't you also need to remove the dependencies from the gemspec? as it is, the gems are still installed since they are listed there.

@mikesea mikesea deleted the optional-dependencies branch March 27, 2024 15:48
@mikesea
Copy link
Contributor Author

mikesea commented Mar 27, 2024

It depends how you're using this plugin. We're invoking it via prettier in npm, so the Ruby dependencies need to be installed manually. But that's a good point, if you're using this as a gem, these dependencies should be marked as optional in the gemspec for them to not get installed.

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

3 participants