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

Add bun support #12

Merged
merged 5 commits into from
Nov 29, 2023
Merged

Add bun support #12

merged 5 commits into from
Nov 29, 2023

Conversation

megatux
Copy link
Contributor

@megatux megatux commented Nov 16, 2023

Related to issue #10

@megatux megatux self-assigned this Nov 16, 2023
@megatux megatux force-pushed the issue-10__add_bun_support branch 2 times, most recently from 0251ed1 to 9cc1553 Compare November 16, 2023 22:03
@megatux megatux marked this pull request as ready for review November 16, 2023 22:27
Copy link

@blacksam07 blacksam07 left a comment

Choose a reason for hiding this comment

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

Can we update the desc method in the line 10 of install_generator.rb file and include the reference to bun

else
create_file manifest, SPROCKETS_SETUP
end
add_to_manifest(manifest('app/javascript'), IMPORTMAP_SETUP)

Choose a reason for hiding this comment

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

Should be BUN_SETUP or we will use the same as importmap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a new constant

@@ -11,7 +11,9 @@ class InstallGenerator < ::Rails::Generators::Base

# Setup HTMX
def setup
if importmap?
if bun_configured?

Choose a reason for hiding this comment

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

To follow up the method naming style, can we rename to bun?

Copy link
Collaborator

@JulianPasquale JulianPasquale left a comment

Choose a reason for hiding this comment

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

Looks good!

CI is failing but I don't think that is related to this PR

Copy link

@blacksam07 blacksam07 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@megatux
Copy link
Contributor Author

megatux commented Nov 27, 2023

Looks good!

CI is failing but I don't think that is related to this PR

Right, seems related to this Truffleruby issue

@JulianPasquale JulianPasquale merged commit 43dc98d into master Nov 29, 2023
28 of 32 checks passed
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