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

Skip dispatching messages if Turbo is not loaded #77

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

trevorrjohn
Copy link

Spark raises errors on pages where Turbo is not loaded. We now check to see if Turbo is loaded before trying to action on the messages.

My Use Case

I have a Rails app where I am using Turbo on the majority of pages, but specific pages are using React. Spark raises errors in my JS console when I am on those React pages.

Considerations

I wasn't sure if this should be a console.log to warn people or use the internal log function. I went with the internal log so that it wouldn't be noisy for people who know what they are doing. However I could see this as being confusing for first time users who maybe don't have Turbo loaded and don't understand why it isn't working.

Copy link
Contributor

@codergeek121 codergeek121 left a comment

Choose a reason for hiding this comment

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

I had a quick look and added some suggestions 👍

app/assets/javascripts/hotwire_spark.js Show resolved Hide resolved
app/assets/javascripts/hotwire_spark.js Outdated Show resolved Hide resolved
app/assets/javascripts/hotwire_spark.js Outdated Show resolved Hide resolved
@trevorrjohn
Copy link
Author

@codergeek121 I adopted your suggestions and it should be good now.

@@ -1569,6 +1569,10 @@ var HotwireSpark = (function () {
return new ReplaceHtmlReloader().reload();
}
async reload() {
if (!window.Turbo) {
console.log(`[hotwire-spark] Tried to replace the page with Turbo, but Turbo is not available on window.Turbo`);
Copy link
Contributor

Choose a reason for hiding this comment

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

A small nitpick to align with the existing logger:

Suggested change
console.log(`[hotwire-spark] Tried to replace the page with Turbo, but Turbo is not available on window.Turbo`);
console.log(`[hotwire_spark] Tried to replace the page with Turbo, but Turbo is not available on window.Turbo`);

I'd probably extract to logger.js and extract a const TAG = "[hotwire_spark]".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants