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

Delete Unused JS and Template files #1611

Merged
merged 2 commits into from
Dec 22, 2022

Conversation

klenotiw
Copy link
Contributor

@klenotiw klenotiw commented Dec 13, 2022

What this PR does

fixes: #1595

removes:

* app/views/shopify_app/sessions/enable_cookies.html.erb
* app/views/shopify_app/sessions/request_storage_access.html.erb
* app/views/shopify_app/sessions/top_level_interaction.html.erb
* app/views/shopify_app/shared/post_redirect_to_auth_shopify.html.erb

* app/assets/javascripts/shopify_app/itp_helper.js
* app/assets/javascripts/shopify_app/storage_access.js
* app/assets/javascripts/shopify_app/enable_cookies.js
* app/assets/javascripts/shopify_app/partition_cookies.js
* app/assets/javascripts/shopify_app/post_redirect.js
* app/assets/javascripts/shopify_app/request_storage_access.js
* app/assets/javascripts/shopify_app/storage_access_redirect.js
* app/assets/javascripts/shopify_app/top_level_interaction.js
* app/assets/javascripts/shopify_app/top_level.js

I also had to delete tests related to

* app/assets/javascripts/shopify_app/itp_helper.js
* app/assets/javascripts/shopify_app/storage_access.js

Reviewer's guide to testing

Things to focus on

  1. Are we cool with deleting the files?

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 force-pushed the klenotiw/remove-unused-js-and-template-files branch from 99e91f8 to 761d589 Compare December 13, 2022 19:12
@klenotiw
Copy link
Contributor Author

Ok everything seem to work at this point in the commit history. Im going to look at removing these files and corresponding tests.

* app/assets/javascripts/shopify_app/itp_helper.js
* app/assets/javascripts/shopify_app/storage_access.js

@klenotiw
Copy link
Contributor Author

Didn't have any trouble making a new app with the template or generating from shopify_app

@klenotiw klenotiw marked this pull request as ready for review December 13, 2022 20:57
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.

I'm fairly certain this is a safe proposal, but as a safeguard would you double check these functions aren't being used anywhere in the codebase? We also need to be sure to be ready to release a breaking change of this gem with this change, so I'd hold off on merging until closer to release date.

@klenotiw
Copy link
Contributor Author

Ok I took another pass and wasn't able to find any use of these functions in the template or shopify_app

@klenotiw klenotiw merged commit 8c18e68 into main Dec 22, 2022
@klenotiw klenotiw deleted the klenotiw/remove-unused-js-and-template-files branch December 22, 2022 15:34
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems January 5, 2023 21: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.

Unused template and JavaScript files?
2 participants