Skip to content
This repository has been archived by the owner on Jan 30, 2023. It is now read-only.

➖ Remove the need for external safe compare #26

Merged
merged 4 commits into from
Oct 15, 2020

Conversation

amorriscode
Copy link
Contributor

Description

Fixes #2

The safe-compare was an unneeded external dependency. This PR removes it and adds a file to do it without new deps.

To test locally I built a new Shopify App and started a local dev server. I was able to install the app on my test store.

Type of change

  • Patch: Bug/ Documentation fix (non-breaking change which fixes an issue or adds documentation)
  • Minor: New feature (non-breaking change which adds functionality)
  • Major: Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have added a changelog entry, prefixed by the type of change noted above

src/auth/safe-compare.ts Outdated Show resolved Hide resolved
src/auth/safe-compare.ts Show resolved Hide resolved
@amorriscode
Copy link
Contributor Author

Thanks for catching my goofs @mkevinosullivan! 🙏

@mkevinosullivan
Copy link
Contributor

👋 Hi Anthony, I was just about to merge in the changes and create a release until I noticed there are no unit tests for the safe-compare.ts file.

When you get a chance, can you create src/auth/test/safe-compare.test.ts and add a few unit tests please?

@amorriscode
Copy link
Contributor Author

@mkevinosullivan I added a test but had a question. Although TypeScript is being used, safeCompare doesn't have protection again non-string input during runtime. I didn't see any guards in the code against things like that so just thought I'd ask if you want that handled?

@mkevinosullivan
Copy link
Contributor

@mkevinosullivan I added a test but had a question. Although TypeScript is being used, safeCompare doesn't have protection again non-string input during runtime. I didn't see any guards in the code against things like that so just thought I'd ask if you want that handled?

It's generally a good idea, but I think it's OK given the limited usage here.

Thank you for the contribution!

@mkevinosullivan mkevinosullivan merged commit 7fcdf66 into Shopify:master Oct 15, 2020
@mkevinosullivan mkevinosullivan temporarily deployed to production October 16, 2020 17:03 Inactive
simonhaenisch added a commit to simonhaenisch/koa-shopify-auth that referenced this pull request Nov 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

safe-compare shouldn't be used in koa-shopify-auth
3 participants