Default profile fields
Closed this issue · 7 comments
According to the messenger docs, the default fields have changed and to access the other fields, one has to ask for access.
From my usage of other bots building libraries/software and also bare development without one, this will throw permission error and we'd have to change the default fields.
I am working on a simple PR to change these --the default fields.
On trying a demo bot, I got this error:
(#100) Tried accessing nonexisting field (first_name) on node type (Page)
I figured its due to this issue. I think that PR should fix things
I experienced this problem and made a change that allows you to request the fields you want (PR #49 ). I kept the defaults so the change doesn't break applications which depends on this package. Change like this should be released with a major version bump. However, if you go ahead with changing this behaviour, I think a better approach would be to get rid of defaults and require applications to specify the fields they want. It would avoid issue like this in the future and if we change the declaration of that method and require fields to be specified it would not break applications by accident when people pull new version. /cc @paked
@pranas: Historically my approach to versioning with this library has been to assume that anyone using it seriously is vendoring it themselves -- and on top of that, providing a list of breaking changes in the README to make upgrading easy.
Obviously, since we've added a go.mod file this isn't how we should do it anymore. I'm not going to lie, I forgot this yesterday! Going forward I'll attempt to be more mindful of how I handle these sorts of changes.
What you've described (removing defaults entirely) seems like the correct way to go about this, especially since it seems that Facebook have a habit of changing these defaults. From an API perspective it would be better to place the onus on developers to ensure they have the right permissions for each field they want: which I believe manually specifying them does.
Would you be able to submit a PR to add this functionality? (or maybe @OAyomide?).
In terms of versioning, once we've made that change I think we should tag a v1.0.0 release immediately so we're properly ready for Go modules. Alternatively we could tag v1.0.0 now and then another breaking change (I suppose it would be v2.0.0? afterwards...)
I see @OAyomide wants to contribute so he/she could do it (I could pick it up otherwise). The solution I see is changing
ProfileByID(id int64, profileFields ...string)
to
ProfileByID(id int64, profileFields []string)
and cleaning up the references to defaults. This would break the build after pulling new version and force to update applications rather than silently changing defaults and possibly breaking applications in a weird way which might even slip unnoticed.
Regarding versioning, it would be nice to tag before the change so people can lock at it and upgrade when ready.
@pranas yes! I am quite psyched about contributing as I am working on a Bot framework and this module has been extremely helpful 🙃
About the solution, Makes perfect sense to me.
I'll work on it submit the PR.
And its a He 😄
RE versioning:
I've just tagged a version v0.0.1 on master.
Once #51 gets merged I will tag v1.0.0.
Thanks for your help everyone!
Done.
Let me know if you have any issues :)