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

Migrate to N-API (fixes #6) #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Jule-
Copy link

@Jule- Jule- commented Dec 18, 2019

As mentioned it fixes #6, in other words it is working with Node V12.

@nikeee
Copy link

nikeee commented Mar 18, 2020

Is there anything that prevents this from merging?

@Jule- Jule- mentioned this pull request Mar 18, 2020
@Jule-
Copy link
Author

Jule- commented Mar 18, 2020

I don't know, no sign from @jorangreef since I have created it while I see that he continues telling people that he will do the N-API migration soon, likely by himself. I am disappointed, I tried to help...

@jorangreef
Copy link
Contributor

Thanks @Jule- ! Sorry, somehow I missed this.

What I meant by moving to N-API, was moving to the pure C N-API, i.e. no more C++.

@Jule-
Copy link
Author

Jule- commented Mar 18, 2020

@jorangreef no problem it happens. 😉

Why do you want to go for pure C N-API ? Is C++ node-addon-api not enough?

@jorangreef
Copy link
Contributor

It's pure C :)

@edmundo096
Copy link

@jorangreef Do you think that we could use Jule-'s changes meanwhile the C version gets through?

I used Jule-'s commit (with npm i https://github.com/Jule-/utimes.git#napi-migration) and all went good enough, working correctly on node v12.3.1 on Win x64.

Off-topic: I see @jorangreef you have the @ronomon/queue as a dependency. I think this is used only by the testing script, so it could be moved to be a devDependency (npm i -D @ronomon/queue should do the trick).
This would make the package to have only 1 dependency, the node-addon-api.

Off-topic 2: Like to your profile pic @Jule- !

Great compact utility library by the way

@jorangreef
Copy link
Contributor

Thanks @edmundo096 , I don't want to push a new release unless it's N-API, and you should be able to cherry pick that PR if needed?

Thanks for the dev dependency tip. I will do that in the next N-API release. I hope to get to this soon!

@Jule-
Copy link
Author

Jule- commented Apr 14, 2020

I do not understand why not releasing a intermediate version...

Whatever, it could be interesting to see the pure C improvement and which application could benefit of this C implementation performance upgrade if it really exists. I am not an expert on the subject but IMHO you will hit drives bottleneck way before the C++ overhead if you are trying to batch parallelized file changes. Just curious.

Off-topic: Thanks @edmundo096! 😅

@jorangreef
Copy link
Contributor

Thanks @Jule-,

It's not for performance. C is cleaner, especially using the N-API C library.

@Jule-
Copy link
Author

Jule- commented Apr 14, 2020

Ok @jorangreef, looking forward to see your implementation! 🙂

Thanks again for this useful lib! 👍

@Offirmo
Copy link

Offirmo commented Jun 9, 2020

Any news?

@jlindberg-oss
Copy link

Has anyone tried https://github.com/baileyherbert/utimes? Looks like it's trying to be a fork of this lib to support modern node versions.

@Jule-
Copy link
Author

Jule- commented Sep 14, 2020

@jlindberg-oss nope, didn't test it but it seems he has took my compatibility changes so it should be ok.

@baileyherbert thanks for doing it, I have been so close to do it... 😅
And thanks for the credit! 😉

@Offirmo
Copy link

Offirmo commented Sep 2, 2021

@jlindberg-oss I just tried https://github.com/baileyherbert/utimes and it works on latest macOs 👍

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.

Build fails with Node 12/Win
6 participants