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

Pass notifierOptions every were that notifier is called. #155

Merged
merged 4 commits into from
Oct 7, 2020

Conversation

dallinbjohnson
Copy link
Collaborator

@dallinbjohnson dallinbjohnson commented Oct 5, 2020

Summary

This PR allows you to pass notifierOptions for all actions that call the notifier function. sendResetPwd action had this already implemented.

(If you have not already please refer to the contributing guideline as described
here
)

  • Tell us about the problem your pull request is solving.
  • Are there any open issues that are related to this?
  • Is this PR dependent on PRs in other repos?

If so, please mention them to keep the conversations linked together.

Other Information

If there's anything else that's important and relevant to your pull
request, mention that information here. This could include
benchmarks, or other information.

Your PR will be reviewed by a core team member and they will work with you to get your changes merged in a timely manner. If merged your PR will automatically be added to the changelog in the next release.

If your changes involve documentation updates please mention that and link the appropriate PR in feathers-docs.

Thanks for contributing to Feathers! ❤️

Copy link
Contributor

@bjohnson-ionrev bjohnson-ionrev left a comment

Choose a reason for hiding this comment

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

I agree with the approach, although I'm not seeing any changes or additions to unit tests

src/service.js Show resolved Hide resolved
src/send-reset-pwd.js Show resolved Hide resolved
@bjohnson-ionrev bjohnson-ionrev merged commit 7ecefc6 into master Oct 7, 2020
@claustres
Copy link
Collaborator

I've seen that you made a release after merging this PR. There are actually dedicated scripts in the module to do so, you need to run eg npm run release:patch (or minor/major). This will automatically increase the version number, push to NPM but as well push a tag to Github and update the changelog (you need to install https://github.com/github-changelog-generator/github-changelog-generator).

We should probably add something in the docs about that. Let me know.

@dallinbjohnson
Copy link
Collaborator Author

dallinbjohnson commented Oct 13, 2020

@claustres That was a mistake I made. I actually ran npm run release:patch but I had a bad configuration with my IDE so it errored out but it still incremented my local git. I push up some change to try to undo what I did. Then we figured out what happened and ran that command again and fixed it. I also installed github_changelog_generator so I am not sure why that did not work.

Ya adding more info in the docs would be great!

@claustres
Copy link
Collaborator

Will try to fix it, would also be great to fill the PR description to know what problem is solved.

@claustres
Copy link
Collaborator

I've just added this. Hope this will help.

@fratzinger fratzinger deleted the notifierOptions branch September 22, 2022 09:18
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