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

Generated App doesn't work in Safari/Firefox #1506

Merged
merged 7 commits into from
Sep 19, 2022

Conversation

klenotiw
Copy link
Contributor

@klenotiw klenotiw commented Sep 13, 2022

What this PR does

The index.html.erb had a messed up import. The root of the problem seems to still be a mystery, but there is a problem with how shims work in Rails 7. I found that if I remove the event listener and use import, instead of import() then everything seems to work.

Q: Why do we need to remove the event listener?
The only real answer I have is that the event is already gone after the shim gets loaded. I figured this out with console logs but forgot to screenshot it. If this seems weird and you want me to get another screenshot I can.

closes Embedded apps not working on some browsers

Reviewer's guide to testing

Honestly the best way to test this is create an app in rails 6 and 7. If you have a previous app I think you should be able to use rails generate shopify_app on this branch, and your index.html.erb file should change slightly.

Things to focus on

  1. Is it ok to use two separate templates?
  2. Is there anything to be concerned about by removing the event listener
  3. Is there anything to be concerned about by using import instead of import()

Checklist

Before submitting the PR, please consider if any of the following are needed:

  • Update CHANGELOG.md if the changes would impact users
  • Update README.md, if appropriate.
  • Update any relevant pages in /docs, if necessary
  • For security fixes, the Disclosure Policy must be followed.

@klenotiw klenotiw marked this pull request as ready for review September 15, 2022 14:08
Copy link
Contributor

@nelsonwittwer nelsonwittwer left a comment

Choose a reason for hiding this comment

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

Thanks for tracking this down and getting this fix in!

Copy link
Contributor

@paulomarg paulomarg left a comment

Choose a reason for hiding this comment

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

Just a nit

Comment on lines 51 to 53
document.addEventListener("DOMContentLoaded", async function() {
displayProducts();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we simplify this?

Suggested change
document.addEventListener("DOMContentLoaded", async function() {
displayProducts();
});
document.addEventListener("DOMContentLoaded", displayProducts);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right! I was trying to do this but I was using displayProducts() instead of displayProducts lesson learn lol. Thanks for this!

@klenotiw klenotiw merged commit 196527a into main Sep 19, 2022
@klenotiw klenotiw deleted the klenoitw/generated-app-works-in-all-browsers branch September 19, 2022 20:11
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems October 3, 2022 13:04 Inactive
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