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

Changes the default for preloading pins #211

Closed
wants to merge 1 commit into from

Conversation

codergeek121
Copy link

Before this PR, you'd have to opt-in to preloading pins. Now you have to opt out for some pins.

If this PR is too big of a breaking change, I could also introduce a setting, which can be toggled in a major release:

# engine.rb
config.importmap.preload_by_default = true

I'd be happy to adapt 👍

@dhh
Copy link
Member

dhh commented Dec 21, 2023

Awesome. Yes I don’t want to change a default and surprise people. Let’s add the toggle and we can default that in next Rails. Thanks!

@codergeek121
Copy link
Author

I adapted the PR as discussed to just include a config for the preloading default without changing the current behavior.

A little bit OT:
I struggled a bit with the testsuite, because one service had connectivity issues. If webmock is a thing that this gem would adapt for the tests, I could look into integrating it and try to make the suite more reliable.

@dhh
Copy link
Member

dhh commented Jan 1, 2024

On second thought, there's a bunch of defaults I want to change for version 2.0, so I've just rolled this in with that in its pure form: #218

@dhh dhh closed this Jan 1, 2024
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.

2 participants