Skip to content

Conversation

haakemon
Copy link
Contributor

The previous PR (#54) was closed on incorrect assumptions. I added a comment on it, but got no response, so I hope a new PR is fine. We use this library, and the incorrect typings are annoying and I would like to see it fixed :)

So, let me try and clear up the confusion from the previous PR:

There is a get (https://github.com/janl/node-jsonpointer/blob/main/jsonpointer.js#L66) function that accepts 2 parameters, and
there is a set (https://github.com/janl/node-jsonpointer/blob/main/jsonpointer.js#L79) function that accept 3 parameters

Additionally, on the compile object
there is also a get (https://github.com/janl/node-jsonpointer/blob/main/jsonpointer.js#L89) method that accepts 1 parameter, and
there is also a set (https://github.com/janl/node-jsonpointer/blob/main/jsonpointer.js#L92) method that accepts 2 parameters.

Currently in the typings, both variants accept 2 and 3 parameters, which is incorrect.

This is also demonstrated in the tests, f.ex here: https://github.com/janl/node-jsonpointer/blob/main/test.js#L14 and here, on the compile object where only 1 parameter is used for get: https://github.com/janl/node-jsonpointer/blob/main/test.js#L126 and 2 parameters is used for set: https://github.com/janl/node-jsonpointer/blob/main/test.js#L127

This PR fixes the incorrect typings for the get/set methods on the compile object.

Thanks for the great library, I hope you will consider accepting this fix :)

@marcbachmann
Copy link
Collaborator

This looks good. Thanks for bringing this up again.

@marcbachmann marcbachmann merged commit b8e1e6a into janl:main Jan 21, 2022
@marcbachmann
Copy link
Collaborator

I'd like to release that once #50 gets merged

@haakemon haakemon deleted the fix-typings branch January 22, 2022 01:18
@haakemon
Copy link
Contributor Author

Hi @marcbachmann , would it be possible to get this released without waiting for #50? There has not been any progress on that issue for some months now, and this typing issue is still bugging me 🙂

This was referenced Jul 13, 2022
@marcbachmann
Copy link
Collaborator

Released as v5.0.1

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.

2 participants