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

TypeScript refactor #97

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

TypeScript refactor #97

wants to merge 13 commits into from

Conversation

halvardssm
Copy link

@halvardssm halvardssm commented Oct 1, 2024

Hi! I was practicing some refactoring patterns in relation to hacktoberfest and accidentally rewrote jmespath.js to jmespath.ts. The runtime is Deno for simplicity, but the published package can be loaded in any runtime as this is general TS (besides the bin.ts). I already published it here under my namespace on JSR, but if you accept this PR, I would recommend to create a new namespace for jmespath, and publish it there.

Although this PR is large, it is not that complicated. The easiest way to look through, is to look at each commit in order. then you will see a step by step change with minimal adjustments to the original code flow.

If you have any questions or concerns, please let me know, and I'll be happy to adjust :)

@springcomp
Copy link

@halvardssm that is very interesting.

I also help maintain a typescript implementation of that library in our JMESPath Community edition.

Is there anything specific to make it work with Deno ?

@halvardssm
Copy link
Author

Hi @springcomp !

I have to say that this refactor really happened accidentally. I was not planning on rewriting the whole library, so I didn't do much research beforehand in regards to existing libraries with TS support.

After looking through your library, I don't think that there is anything which does not allow it to work with Deno, but I haven't tested it. If you want to, I can also submit the pr to that library instead if that is better. I can also add a transpiling task to generate the js and types. The good thing about this implementation is that it does not require any dependencies (it only has dependencies for tests).

Let me know if you want me to take any actions 😊

@springcomp
Copy link

springcomp commented Oct 28, 2024

@halvardssm in fact, following a quick Getting Started guide, I was able to demonstrate that this library, indeed, works from Deno.

// @ts-check
import * as jmespath from "npm:@jmespath-community/jmespath";

const result = jmespath.search({hello: "world"}, "hello");
console.log(result);

My question now becomes, is there an interest to publish that package on https://jsr.io/ ? What would be the pros and cons ?
To that end, what does the work look like for me ?

I’m not sure I would invest the time to completely overhaul the project and make it Deno-native, although I suspect the scope of the work would be limited to the unit-tests only. Besides, I publish ESM as well as CommonJS versions of the library to maximize the compatibility on existing projects.

Please, tell me what you think ?

@halvardssm
Copy link
Author

That's great! Then not much work is needed to publish it to JSR 👍🏻

So to answer your question, yes there is interest in having this library on JSR as well. It is simply an alternative to NPM that gives better support for typings and cross runtime compatibility.

In reality, the jmespath code does not need to be for any specific runtime, it can be standard TS/JS. My branch only uses Deno for tooling to reduce dependencies and ease development. Although my preference is Deno due to its easy of development and built in tooling, I won't push hard for a rewrite, but you can take what you want from my branch.

To publish the existing code on JSR, you only need a jsr.json or deno.json file (if you want to keep it node, I would recommend jsr.json, and if you want to switch to deno I would recommend deno.json, either way you can change this later), i can help you with this part if you want. Let me know if you want me to create the namespace and transfer it to you, or you can create it and if you want my help setting it up, you can add me to the namespace as a member.

As a note, if you are interested in moving to deno, I would agree that the scope of deno specific code would be the tests, and to generate npm compatible code we could use dnt or esbuild, both should work fine. My branch should also be compatible with your implementation with minor adjustments, but again i won't push hard for adopting it.

Let me know which direction you want to go!

@springcomp
Copy link

@halvardssm thanks for the detailed explanations.

I will look at those options.
I think using jsr.json is indeed the good way to go for now.

I’m thinking that, in order to promote contibutions, maybe I can also try and setup a deno-native project structure. I think I will have to see what are the impacts on the tests – pretty sure they should be minimal, in fact.

Thanks for your help, that already really appreciated.

@halvardssm
Copy link
Author

If you would like the help, I can try to create a deno refactor of your repo, as it shouldn't take long. It would have to be next week however as i am on a trip this week 😊

@springcomp
Copy link

If you would like the help, I can try to create a deno refactor of your repo, as it shouldn't take long. It would have to be next week however as i am on a trip this week 😊

That’s too generous. I cannot ask that from you.
Especially as I’m not sure what, if any, direction I’d like to go.

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