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

Update to build for Node.js v12 #271

Closed
wants to merge 1 commit into from

Conversation

fengmk2
Copy link
Contributor

@fengmk2 fengmk2 commented Apr 24, 2019

No description provided.

@fengmk2 fengmk2 changed the title Update to build for node v12 Update to build for Node.js v12 Apr 24, 2019
@fengmk2
Copy link
Contributor Author

fengmk2 commented Apr 24, 2019

Need nan support Node.js 12 first nodejs/nan#849

@pipobscure
Copy link
Contributor

Can you also bump the version in package.json please.

@fengmk2
Copy link
Contributor Author

fengmk2 commented Apr 24, 2019

@pipobscure sure, now We have to wait till the next release of nan with support of node v12.x.

@sam-github
Copy link

I wonder if using napi instead of nan would allow faster sync with node releases?

@Flarna
Copy link

Flarna commented Apr 26, 2019

Current NAN version (2.13.2) is already supporting Node.JS 12; only part missing is update of readme but this should not block.
There are several places in fsevents@1.x where v8 API is used directly which should be changed to NAN calls.

@pipobscure
Copy link
Contributor

@sam-github v2 is NAPI update to that please. Only downside is there’s no v6.x support and older v8.x releases aren’t supported either.

@sam-github
Copy link

@pipobscure I missed that this was a maintenance release of an older line, I'm glad you are maintaining v1, thanks, and I see why it is still nan-based. (And I don't use personally use fsevents, I work on maintaining node, I'm just keeping an eye on fallout from our 12.x release).

@felixfbecker
Copy link

So what is blocking this?

@pipobscure
Copy link
Contributor

@felixfbecker nothing other than the fact that i have a full-time job, a family and a bunch of commitments. I’ll need to figure out what exactly the problem is and then fix it. If you want to have a go, I’m more than happy to look at pull-requests.

@felixfbecker
Copy link

Looks like Node 12 removed v8::Handle which fsevents uses facebook/create-react-app#6891 (comment)

Unfortunately I am in the same boat as you but also have zero experience with native addons / C++.

@kroleg
Copy link

kroleg commented Apr 29, 2019

Made enough changes to compile without errors #274 Tests are passing (at least in v12 on my machine)
I am not a C guy, so not sure if FSEvents::Initialize should be more like plusOne example in https://nodejs.org/api/addons.html#addons_wrapping_c_objects

@pipobscure
Copy link
Contributor

Superceded by #274

@pipobscure pipobscure closed this Apr 29, 2019
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.

None yet

6 participants