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

enhance: Add axios to hook function #3001

Merged
merged 1 commit into from
Jun 1, 2022

Conversation

Maarrk
Copy link
Contributor

@Maarrk Maarrk commented May 28, 2022

Community PR Review Checklist

First Time Specifics

  • if its your first pull request to Dendron, watch out for the CLA bot that will ask you to agree to Dendron's CLA
  • if its your first pull request and you're on our Discord, make sure that Kevin gives you the horticulturalist role by adding your discord as part of the description 👨‍🌾👩‍🌾
    • Maarrk#9537
  • (optional) ping @Dendron Team in the #dev channel of our discord - we usually respond to PRs within 24h

Commit

  • make sure your branch names adhere to our branch style
  • make sure the commit message follows Dendron's commit style
    • I didn't see a section named "commit style" after visiting that note
    • I just looked at other commits
  • if this pull request is addressing an existing issue, make sure to link this PR to the issue that it is resolving.

Code

  • code should follow Code Conventions
  • circular dependency check: make sure your code is not introducing new circular dependencies in plugin-core. See Avoiding Circular Dependencies.
    • only importing axios package which was already a dependency, won't be circular because it's external
  • sticking to existing conventions instead of creating new ones
    • added axios in the same way as execa

Tests

Docs

  • if your change reflects documentation changes, also submit a PR to dendron-site and mention the doc PR link in your current PR
    • not needed, this is self-documenting with the hook template
  • does this change introduce a new or better way of doing things that others need to be aware of? if so, an async should be created and a process added in Development or Packages
    • see above

@CLAassistant
Copy link

CLAassistant commented May 28, 2022

CLA assistant check
All committers have signed the CLA.

@Maarrk Maarrk force-pushed the enhance/hook-function-http branch from a182b80 to f194424 Compare May 31, 2022 09:41
@Maarrk
Copy link
Contributor Author

Maarrk commented May 31, 2022

Issues with testing:

  • There is a test to verify if an axios object is passed in, but it fails
    • I know that axiosHookPayload actually runs, if I put something else there
    • It errors anytime I reference any axios properties
    • However it is passed properly if used normally (not in test). Is there another runner for hooks during tests that's not in hooks.ts?
  • I'm not sure if I should do some actual HTTP requests in the test, probably not
    • Is there some other test I could do?

@kevinslin
Copy link
Member

when I step through this, it looks like axios is returning the following hooked body application/json' also, heads up that you can step through with debugger, see https://wiki.dendron.so/notes/2ipygd3ly9aum5f7zgy6dy4

@Maarrk Maarrk closed this Jun 1, 2022
@Maarrk Maarrk force-pushed the enhance/hook-function-http branch from f194424 to c540ff2 Compare June 1, 2022 15:52
@Maarrk
Copy link
Contributor Author

Maarrk commented Jun 1, 2022

The test passes now, I rebased the commit to be on top of current master.

If there aren't other ideas for tests, this is ready to merge.


The PR was closed by mistake, because i did git commit without the -a switch 🤦 , and I needed to force push to keep the PR a single commit

@Maarrk Maarrk reopened this Jun 1, 2022
@Maarrk Maarrk marked this pull request as ready for review June 1, 2022 16:09
@auto-assign auto-assign bot requested review from bpathakota and kevinslin June 1, 2022 16:09
@kevinslin kevinslin merged commit 697c1c6 into dendronhq:master Jun 1, 2022
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.

4 participants