baudehlo / node-fs-ext

Extras missing from node's fs module
MIT License
101 stars 45 forks source link

Use Nan v2 #50

Closed ggreer closed 8 years ago

ggreer commented 8 years ago

This PR updates fs-ext to work with Nan v2. This is needed for compatibility with io.js v3 and the soon-to-be-released node.js v4. Tests pass on OS X 10.10.5, using io.js v3.2.0. Sorry for the huge number of changes, but Nan v2's API is quite different from v1.

If you prefer, I can squash these commits. I'm never sure if maintainers like clean commits or if they want more insight into how the code was made.

baudehlo commented 8 years ago

How far back does the backcompat of Nan2 go?

On Aug 30, 2015, at 7:50 PM, Geoff Greer notifications@github.com wrote:

This PR updates fs-ext to work with Nan v2. This is needed for compatibility with io.js v3 and the soon-to-be-released node.js v4. Tests pass on OS X 10.10.5, using io.js v3.2.0. Sorry for the huge number of changes, but Nan v2's API is quite different from v1.

If you prefer, I can squash these commits. I'm never sure if maintainers like clean commits or if they want more insight into how the code was made.

You can view, comment on, or merge this pull request online at:

https://github.com/baudehlo/node-fs-ext/pull/50

Commit Summary

Update dep to Nan 2.x. Use Nan 2.x API. Still need to fix NanAssignPersistent. Hooray, it compiles. Return after setting return value for sync methods. Fixes tests. Cleanup. Prefer methods to explicit function invocations. You never know when someone might use inheritance later. File Changes

M fs-ext.cc (236) M package.json (2) Patch Links:

https://github.com/baudehlo/node-fs-ext/pull/50.patch https://github.com/baudehlo/node-fs-ext/pull/50.diff — Reply to this email directly or view it on GitHub.

ggreer commented 8 years ago

The Nan README says it works with versions as old as Node v0.8, but I haven't tested it. Maybe some Travis CI tests would be a good idea? I can submit a PR for that, but someone with push access would have to enable the hooks.

ChALkeR commented 8 years ago

Adding to the list: https://github.com/nodejs/node/issues/2798.

pmurias commented 8 years ago

why will this need to be undone for V8 4.7?

baudehlo commented 8 years ago

Since there's no answer to this question, I'm just going to merge and release this.

ggreer commented 8 years ago

Thanks. Can you please tag a new release?