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

Fix record changed() logic for comparing current value Dates #695

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jcao49
Copy link
Contributor

@jcao49 jcao49 commented Nov 19, 2024

We are currently not calculating record.changed correctly for datetime fields as we compare the current value as a Date instance against the previous value which is stored as a string. We need to make sure that in any case where the current/previous is being compared as a Date vs string we convert the string to a Date and compare the actual dates based on that.

@pistachiobaby if you can pls 🎩 :) :
To see the problem:

  1. In production, create a shopify app and add a real connection. Select the Product model
  2. In product update action in the run function, add (before the await save(record) call)
if (record.changed("publishedAt")) {
    logger.info(`in my run function publishedAt was updated to: ${record.publishedAt}`);
}

Add the same snippet in the onSuccess function

if (record.changed("publishedAt")) {
    logger.info(`in my onSuccess function publishedAt was updated to: ${record.publishedAt}`);
}
  1. Trigger a product/update webhook to create the product record with the create action. Trigger the webhook again to actually run the update action
  2. See that the record.changed("publishedAt") condition was false in the run function and no log in it is shown
  3. See that the record.changed("publishedAt") condition was true in the onSuccess function and the log in it is shown even though the publishedAt didn't actually change! <--- this is the bug

To see the fix:

  1. cd into /packages/api-client-core
  2. Run pnpm build and then pnpm pack to generate the tarball
  3. Stop dev if its currently running the main gadget repo
  4. In the main gadget repo, in the root package.json set the @gadgetinc/api-client-core package to point to the newly generated tarball
"@gadgetinc/api-client-core": "file:/Users/jennycao/dev/js-clients/packages/api-client-core/gadgetinc-api-client-core-0.15.36.tgz",

Do the same in UserlandDependencyVersions.ts
9. Run pnpm install
10. run dev again
11. Follow the same steps as before but this time see that record.changed("publishedAt") is correctly false in the onSuccess function

PR Checklist

  • Important or complicated code is tested
  • Any user facing changes are documented in the Gadget-side changelog
  • Any immediate changes are slated for release in Gadget via a generated package dependency bump
  • Versions within this monorepo are matching and there's a valid upgrade path

@jcao49 jcao49 force-pushed the fix_record_changed_date_comparison branch 5 times, most recently from 6a10b17 to c75063a Compare November 20, 2024 15:08
Copy link
Contributor

@jasong689 jasong689 left a comment

Choose a reason for hiding this comment

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

changes look good. will 🎩

packages/api-client-core/spec/GadgetRecord.spec.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@pistachiobaby pistachiobaby left a comment

Choose a reason for hiding this comment

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

🎩 ✅

Published Product Update webhook no longer incorrectly returns true for `record.changed("publishedAt") ✅

Unpublished -> Published product does return true for both scenarios ✅
CleanShot 2024-11-25 at 09 19 10@2x

@jcao49 jcao49 force-pushed the fix_record_changed_date_comparison branch from 16c06ab to b6ee589 Compare November 27, 2024 14:40
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