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

Use Nan v2 #50

Merged
merged 5 commits into from
Oct 2, 2015
Merged

Use Nan v2 #50

merged 5 commits into from
Oct 2, 2015

Conversation

ggreer
Copy link
Contributor

@ggreer ggreer commented Aug 30, 2015

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
Copy link
Owner

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:

#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
Copy link
Contributor Author

ggreer commented Aug 31, 2015

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
Copy link

ChALkeR commented Sep 10, 2015

Adding to the list: nodejs/node#2798.

@pmurias
Copy link

pmurias commented Sep 29, 2015

why will this need to be undone for V8 4.7?

@baudehlo
Copy link
Owner

baudehlo commented Oct 2, 2015

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

baudehlo added a commit that referenced this pull request Oct 2, 2015
@baudehlo baudehlo merged commit beb3c6a into baudehlo:master Oct 2, 2015
@ggreer
Copy link
Contributor Author

ggreer commented Oct 11, 2015

Thanks. Can you please tag a new release?

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.

5 participants