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

fixes #115 #116

Merged
merged 7 commits into from
Dec 3, 2022
Merged

fixes #115 #116

merged 7 commits into from
Dec 3, 2022

Conversation

Harsh-br0
Copy link
Contributor

@Harsh-br0 Harsh-br0 commented Oct 26, 2022

Description

As per docs, getDeviceInterests must accept limit and cursor args to be passed with query parameters in the API. Currently, API returns 100 results since limit implies with 100 as default value in API, so i first redesigned the endpoint caller function here to accept query params as object in params arg and reworked the getDeviceInterests method here to return value as Object that includes cursor field from API response and add limit and cursor to method parameters.
Also I've added a test for params arg in do-request.test.js

Notable Changes

  • getDeviceInterests now returns Object instead of an Array.
  • Internal doRequest function can have params arg in Object to represent query params in URL



potentially fixes #115

@benjamin-tang-pusher
Copy link

Hi, I will test this shortly.

@Harsh-br0
Copy link
Contributor Author

Hi @benjamin-tang-pusher , is there any problem in this PR?
I didn't get any updates regarding this pull since 10 days

@benjamin-tang-pusher
Copy link

Hi, sorry for the late response. When I run the the tests ini your branch with npm run test I get the following two failures:

FAIL  src/do-request.test.js
 ● Handles URL with params
   TypeError: Cannot read properties of undefined (reading '0')

FAIL  src/push-notifications.test.js
 ● Test suite failed to run
   SyntaxError: /Users/benjamintang/Documents/development/node/harsh-bro/src/push-notifications.js: Support for the experimental syntax 'optionalChaining' isn't currently enabled (262:14):

Could you check src/do-request.test.js and check if your changes affected this test?

For optionalChaining I get Add @babel/plugin-proposal-optional-chaining (https://git.io/vb4Sk) to the 'plugins' section of your Babel config to enable transformation. Did you have to do this too?

@Harsh-br0
Copy link
Contributor Author

Hi @benjamin-tang-pusher , so I've fixed all tests and that OptionalChaining issue. The problems were the following -

  • Somehow the jest returns undefined on lastCall in .mock property which isn't the expected behaviour as per docs.

  • I forgot to check version compatibility on package and used a ES2020 feature (OptionalChaining) that is obviously unsupported on ES2015 based package. Apologies for it

@sonologico sonologico merged commit 6a58ad5 into pusher:master Dec 3, 2022
@sonologico
Copy link
Contributor

Thank you!

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.

getDeviceInterests doesn't support pagination as per API docs
3 participants