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

Feature/napi #111

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

Feature/napi #111

wants to merge 30 commits into from

Conversation

ghinks
Copy link

@ghinks ghinks commented Jan 4, 2019

This is a conversion from use of the nan to the node-addon-api ( which is for the napi interface ).
The travis file has been updated for linux, osx and windows. There were issues with unit tests on windows. These pre-dated these changes as I do not think windows was available on travis at the time the module was created.

replacing the time.cc file with

- time_napi.cc
- time_napi.h

files

also using the object wrapper methodology from the workshop at Node Summit.
1 outstanding unit test failing.

binding.gyp changed to handle the gmtOffset in localtime
did not find a standard so 4 spaces is adopted
the Time::time method was not used.
…ms differed.

do not unit test on windows as the original nan code had issues.
@ghinks
Copy link
Author

ghinks commented Jan 4, 2019

Additionally I published locally using verdaccio and used the package locally.

@ghinks
Copy link
Author

ghinks commented Jan 4, 2019

ok, I did not see the appveyor build and will address it.

@ghinks
Copy link
Author

ghinks commented Jan 4, 2019

Nathan, I have checked over the AppVeyor build history and the unit tests have not previously passed on windows. May I suggest that as this has been the case for a long time that this not stop the upgrade from NAN to NAPI. The build on windows does work.
Furthermore the napi interface is really for node 8 onwards and perhaps I should update the readme saying that the [email protected] be used for use with node version prior to 8.

- "7"
- "8"
- "10"
- "node"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this version?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also test 11 for now I think

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, node means the latest node and that presently is 11.6.0.

package.json Outdated
},
"devDependencies": {
"mocha": "*"
}
},
"gypfile": true
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that this is required. npm adds it implicitly to the package metadata with the presence of a binding.gyp file.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checking it out ...
Im sure you are correct.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes indeed so, it was in the first hello-world example for NAPI conversion but in no others from the workshop I looked at. I'll raise a PR on the workshop too.

@ghinks
Copy link
Author

ghinks commented Jan 15, 2019 via email

@ghinks
Copy link
Author

ghinks commented Feb 7, 2019

any chance to look at my responses to your questions?

@ghinks
Copy link
Author

ghinks commented Mar 20, 2019

its still here, build on windows was previously broken too. But I think this PR is still valid.

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