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

NodeJS v12 compatibility #17

Merged
merged 6 commits into from
Apr 23, 2020
Merged

NodeJS v12 compatibility #17

merged 6 commits into from
Apr 23, 2020

Conversation

goooseman
Copy link
Contributor

ref package does not work with NodeJS v12 and will never be: source

This PR updates .nvmrc to NodeJS 12 and uses ref-napi libs family instead of ref to resolve the issue.

Two type definitions are added to the project, which will be removed after this and this PR's will be merged and released.

This change is tested with NodeJS 12.13.0, MacOS 10.13.6 and my Canon 80D. Everything works as before.

The example folder is a broken at the moment, because it attempts to install an old ts-gphoto2-driver version and it fails, because of ref module can not be compiled under NodeJS v12. I suggest to release a 2.4.0 version of this lib and bump the dependency version in examples/package.json after.

@goooseman
Copy link
Contributor Author

The build is failing, because examples/package.json installs "@typedproject/gphoto2-driver": "2.2.3", which uses ref, which fails to build under NodeJS v12.

Will be resolved after release, by bumping the dependency version in examples/package.json

goooseman and others added 5 commits April 23, 2020 09:25
Node is moving to NAPI so everyone and everything is moving to -napi to avoid compatibility issues.
This is required for NodeJS v12 compatibility.
@@ -0,0 +1,65 @@
// Type definitions for ref-array-napi
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good idea :) but, I have a doubt when the npm package will be released. Theses files won't be packaged. It means theses types won't be available when a developer install our package.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think to fix it, you have to move this files in src/@types and import it in index.ts? Why do you think ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try it out right now.

@goooseman
Copy link
Contributor Author

Ohhhh, I didn't see you forced push and I've pushed again.

Can you please make a force push again from your local branch without pulling the changes from remote? This will fix the issue.

@goooseman
Copy link
Contributor Author

BTW, moving @types to src/@types and importing in index.ts works and sounds like a great idea

@goooseman
Copy link
Contributor Author

Ok, I've resolved the git issue and moved @types to src

@Romakita
Copy link
Collaborator

Nice :). I'll merge your PR

@Romakita Romakita merged commit 0a25b19 into master Apr 23, 2020
@Romakita Romakita deleted the fix/node-12 branch April 23, 2020 08:11
@Romakita
Copy link
Collaborator

Release is on going :)

@Romakita
Copy link
Collaborator

Released :)

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