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

Node 6 #21

Merged
merged 19 commits into from
May 11, 2018
Merged

Node 6 #21

merged 19 commits into from
May 11, 2018

Conversation

mischnic
Copy link
Contributor

@mischnic mischnic commented May 8, 2018

Closes #19

  • remove await/async in tools
  • add to ci

@mischnic mischnic changed the title [WIP] Node 4 [WIP] Node 6 May 8, 2018
@mischnic
Copy link
Contributor Author

mischnic commented May 9, 2018

I also refactored functions used in both tools into a seperate file.

What would happen if CI ran with multiple node versions? Would they all get published (or multiple times)?

@parro-it
Copy link
Owner

parro-it commented May 9, 2018

I also refactored functions used in both tools into a seperate file.

You rock!

What would happen if CI ran with multiple node versions? Would they all get published (or multiple times)?

We can filter it:

https://github.com/mafintosh/a-napi-example/blob/c42058fdb2f3d8bc771fe5ac26a8264dd2d900eb/appveyor.yml#L32-L41

https://github.com/mafintosh/a-napi-example/blob/c42058fdb2f3d8bc771fe5ac26a8264dd2d900eb/.travis.yml#L24-L34

@mischnic
Copy link
Contributor Author

mischnic commented May 9, 2018

We can filter it:

Pushed. Take a look at the compile errors with older node versions (but the compiled binary works with older verions).

I also refactored functions used in both tools into a seperate file.

While testing the download tools, I nearly rm-rf-ed my home folder while trying to delete the cache 😄 . I'll have to restore some dot files from my backup...

@parro-it
Copy link
Owner

parro-it commented May 9, 2018

Fun fact: while testing the download tools, I nearly rm-rf-ed my home folder while trying to delete the cache 😄 . I'll have to restore some dot files from my backup...

Beware! We should rest more 🧐
😆😆

@parro-it
Copy link
Owner

parro-it commented May 9, 2018

https://nodejs.org/api/n-api.html#n_api_napi_fatal_exception
https://nodejs.org/api/n-api.html#n_api_napi_open_callback_scope

We should remove that two functions... supported only from node >9

@mischnic mischnic changed the title [WIP] Node 6 Node 6 May 9, 2018
@parro-it
Copy link
Owner

parro-it commented May 9, 2018

https://nodejs.org/api/n-api.html#n_api_napi_fatal_exception
https://nodejs.org/api/n-api.html#n_api_napi_open_callback_scope

We should remove that two functions... supported only from node >9

I'll take care of this, but cannot tonight... will do tomorrow

@parro-it
Copy link
Owner

Removed offending functions.
Remove of callback_scope seem to have no observable effects.
But napi_fatal_exception is essential to trigger a useful uncaught exception.
By now, I replaced it with napi_fatal_error, but this . function call abort and brutally kill the process, so it is sub-optimal.
Maybe it's better to just write exception errors on stderr?

@parro-it
Copy link
Owner

More info here:
nodejs/node#15371
nodejs/node#14697

@parro-it
Copy link
Owner

parro-it commented May 11, 2018

Strangely enough, AppVeyor does not install node 6.14.2, it seems stick with 6.14.1

image

Forcing it:

image

@parro-it
Copy link
Owner

Finally I was able to install the correct node 6 version on AV. Merging...

@parro-it parro-it merged commit 2b7c617 into parro-it:master May 11, 2018
@mischnic mischnic deleted the node-4 branch May 12, 2018 10:05
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