search

feathersjs-ecosystem / feathers-authentication-management

Adds sign up verification, forgotten password reset, and other capabilities to local feathers-authentication
https://feathers-a-m.netlify.app/
MIT License
246 stars 98 forks source link

fix verifyChanges causing 0: {, 1: } during insert and patch #216

Open closertotheend opened 11 months ago

closertotheend commented 11 months ago

Due to fact that most realtional dbs do not support string[] inside single column, people cast verifyChanges to text type. When field is deserialized from db we need to parse it twice. Otherwise Object.assign({}, '{}') will yield {0:'{', 1: '}'}

Related issue: https://github.com/feathersjs-ecosystem/feathers-authentication-management/issues/206

netlify[bot] commented 11 months ago

Deploy Preview for feathers-a-m ready!

Name Link
Latest commit c97b82c1063a0be9ea79d3b078c566cbef994014
Latest deploy log https://app.netlify.com/sites/feathers-a-m/deploys/652d59ed26f9490008dd9f3f
Deploy Preview https://deploy-preview-216--feathers-a-m.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

closertotheend commented 11 months ago

Checked locally with npm link, fix seems to be working

claustres commented 11 months ago

Thanks for this as it seems to annoy many people. Not familiar enough with SQLite though to check if it's a valid workaround, but from a more general point of view I would ask if it would not be more "readable" and less risky (eg avoid any exception if user.verifyChanges is an empty string for some weird reason) with something like this (and ideally with a comment on the previous line to explain why we need it): if (user.verifyChanges === '{}') ...

I will let run the tests anyway to ensure it works.