pusher / push-notifications-web

Beams Browser notifications
MIT License
39 stars 19 forks source link

fixes #115 #116

Closed Harsh-br0 closed 1 year ago

Harsh-br0 commented 1 year ago

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



potentially fixes #115

benjamin-tang-pusher commented 1 year ago

Hi, I will test this shortly.

Harsh-br0 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

sonologico commented 1 year ago

Thank you!