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.

Sign up for GitHub

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

Jump to bottom

Packaging for release v18.0.3 #1344

Merged
merged 1 commit into from Jan 7, 2022
Merged

Packaging for release v18.0.3 #1344

merged 1 commit into from Jan 7, 2022

Conversation

NabeelAhsen
Copy link
Contributor

What this PR does

This is a patch release that includes the following bug fixes:

  • Change regexp to match standard ngrok URLs. #1311
  • Make EnsureAuthenticatedLinks compatible with AppBridge 2.0. #1277
    • Includes the host parameter when redirecting to the splash page in an unauthenticated state.

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.

@NabeelAhsen NabeelAhsen self-assigned this Jan 7, 2022
@NabeelAhsen
Copy link
Contributor Author

cc @kirillplatonov

@kirillplatonov
Copy link
Contributor

@NabeelAhsen I have one more important bugfix for AppBridge 2. Preparing the PR now.

@NabeelAhsen
Copy link
Contributor Author

I'll leave this release as a draft for now

@NabeelAhsen NabeelAhsen marked this pull request as draft January 7, 2022 15:43
@NabeelAhsen NabeelAhsen mentioned this pull request Jan 7, 2022
Closed
@kirillplatonov
Copy link
Contributor

@NabeelAhsen In my fork, I also have ShopifyApp::ShopHost concern (kirillplatonov@b46ccc1) which handles host fetching and stores it in cookies. This allows avoiding passing host param manually to every link. I found this concern very convenient and use it in all of my apps. I think it would be great to backport it to this repo too. What do you think?

@NabeelAhsen
Copy link
Contributor Author

NabeelAhsen commented Jan 7, 2022 •
edited

@kirillplatonov I saw that as well, and thought it would be neat to have something like this! Although I would rethink the long-term costs of relying on cookies, especially for embedded apps.

This gem has a lot of tech debt to pay regarding an over-reliance on cookies. We can't rely on 3p cookies set by embedded apps in the near future, and so the gem's been slowly trying to phase out any more writes performed on cookies within the gem.

For example, I don't know how ShopHost will behave in browsers like Brave or Safari. What does it try to do if it can't find a host param at all?

In any case, I don't think it's a blocker to getting this patch released. Thank you for your contributions 🎉 !

What do you think about this release? Is it ready to deploy?

@kirillplatonov
Copy link
Contributor

@NabeelAhsen Yeah, let's publish this patch. I will open a PR for ShopHost concern so we can discuss all details there.

@NabeelAhsen NabeelAhsen marked this pull request as ready for review January 7, 2022 18:59
@NabeelAhsen NabeelAhsen merged commit 3ef00b7 into master Jan 7, 2022
@NabeelAhsen NabeelAhsen deleted the packaging-for-release-18.0.3 branch January 7, 2022 20:12
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems January 7, 2022 20:27 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pepicrft pepicrft pepicrft approved these changes

@jesalerno84 jesalerno84 jesalerno84 approved these changes

Assignees

@NabeelAhsen NabeelAhsen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@NabeelAhsen @kirillplatonov @pepicrft @jesalerno84