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

Improve V9->v10 Upgrade Docs #1212

Merged
merged 12 commits into from
Sep 12, 2023
Merged

Improve V9->v10 Upgrade Docs #1212

merged 12 commits into from
Sep 12, 2023

Conversation

zzooeeyy
Copy link
Contributor

@zzooeeyy zzooeeyy commented Sep 11, 2023

Description

Add more examples and reference for upgrading this gem from v9.

Tophatting:

New

New dedicated page - "Breaking change notice for version 10.0.0"
Link from README

Existing

Existing docs on v10 upgrade

@zzooeeyy zzooeeyy force-pushed the zoey/improve-upgrade-docs branch from 421e182 to 1cd6d4e Compare September 11, 2023 18:55
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.

Love it! Thanks for taking the time to put this together :)

I wonder if we should include some background on WHY we needed the big rewrite. This was before my time but from what I've been able to gather it largely had to do with 1) removing 3rd party cookies and 2) so we could consolidate code bases for REST resources with other languages. @paulomarg do you have any more history you could share with the class here 🙏

@paulomarg
Copy link
Contributor

paulomarg commented Sep 12, 2023

Love it! Thanks for taking the time to put this together :)

I wonder if we should include some background on WHY we needed the big rewrite. This was before my time but from what I've been able to gather it largely had to do with 1) removing 3rd party cookies and 2) so we could consolidate code bases for REST resources with other languages. @paulomarg do you have any more history you could share with the class here 🙏

That's pretty much on the spot. The biggest changes were:

  1. browsers stopped allowing 3rd party cookies, even after jumping through several ITP hoops, which made the code even more complex and error prone. Session tokens and authenticatedFetch were introduced to make it possible for apps to authenticate requests without depending on cookies, but the gem was very closely tied to rails and how it set up sessions (which was fine before), so we had to detach the library from rails in order to be able to work with both cookies and session tokens.
  2. we also relied on an omniauth-oauth2 strategy, which was fine when cookies were the only option, but it became increasingly awkward as we moved more towards session tokens, which meant rewriting the OAuth process in the gem as a whole (hence the introduction of DB-stored sessions rather than cookie-stored).
  3. most of the feedback we got before was due to the ActiveResource classes falling out of sync with the API because a) we have some endpoints that didn't 100% follow REST convention; b) we had a single class for each resource and manually maintained custom methods that didn't work across API versions.
    Since we were adding support for auto-generated, version-specific resources for other languages, we decided to add them for Ruby too, but those same instances where the API doesn't follow convention would become a problem. Thus we opted for the most explicit option where every method was "custom" rather than just some, so that the resources were always consistent.

@nelsonwittwer
Copy link
Contributor

Thanks @paulomarg ! @zzooeeyy - would you mind mixing in that context into your upgrade doc so the community understands why we did what we did? Thank you both!

@zzooeeyy zzooeeyy merged commit a2ad749 into main Sep 12, 2023
8 checks passed
@zzooeeyy zzooeeyy deleted the zoey/improve-upgrade-docs branch September 12, 2023 15:25
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems October 10, 2023 16:44 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.

3 participants