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

improved dependencies directive description #108

Merged
merged 2 commits into from
Dec 18, 2021

Conversation

bertBruynooghe
Copy link
Contributor

Make it more obvious that the javascript comment block is necessary to get dependency loading working. (Especially when not using .js.erb files.)

Make it more obvious that the javascript comment block is necessary to get dependency loading working. (Especially when not using .js.erb files.)
@PikachuEXE
Copy link
Collaborator

I think the original description should remain unchanged (or say there are pitfalls) and mention the pitfalls below with your example code <!-- /* rails-erb-loader-dependencies ... */ -->

@bertBruynooghe
Copy link
Contributor Author

I took the liberty to change the original description slightly ('javascript comment block'), as that makes it clear that // rails-erb-loader-dependencies... doesn't work either.

As to including <!-- /* rails-erb-loader-dependencies */ -->: that only works for html files, while <%# /* rails-er-loader-dependencies %> works more universally (e.g. .json.erb). Adding more examples only adds to the confusion.

That being said, I still think leaving the comment signs out of the detection would make more sense than adapting the documentation, as the current implementation is a violation of the Principle of Least Astonishment, at the heart of ruby and rails.

@PikachuEXE PikachuEXE merged commit ac13b26 into usabilityhub:master Dec 18, 2021
@PikachuEXE
Copy link
Collaborator

Ya just think updating the README is a faster improvement
Let's keep #107 open and see if further improvements can be made

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