Skip to content

Add node-addon-api async iterator example#195

Merged
mhdawson merged 3 commits intomainfrom
async-iterator-example
May 30, 2022
Merged

Add node-addon-api async iterator example#195
mhdawson merged 3 commits intomainfrom
async-iterator-example

Conversation

@KevinEady
Copy link
Copy Markdown
Contributor

@KevinEady KevinEady commented May 14, 2022

An example as requested by nodejs/node-addon-api#1171

@KevinEady
Copy link
Copy Markdown
Contributor Author

cc: @vmoroz @JckXia @deepakrkris

@KevinEady KevinEady force-pushed the async-iterator-example branch from 7552703 to d70e386 Compare May 17, 2022 19:26
@KevinEady
Copy link
Copy Markdown
Contributor Author

Hi team,

I realized that the first example I wrote is a bit contrived and probably not useful in real-world situations. I've created a new example that uses threadsafe functions which I think provides a better use-case in d70e386.

Copy link
Copy Markdown
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

LGTM 👍

}

private:
int _from;
Copy link
Copy Markdown
Member

@legendecas legendecas May 27, 2022

Choose a reason for hiding this comment

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

Nit: we may prefer to suffix with the underscore.

Suggested change
int _from;
int from_;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@KevinEady let us know what you think on this one.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @legendecas / @mhdawson ,

It looks like the standard used in napi.h for non-public members is to prefix with underscore (46 occurrences) versus suffix (1 occurrence), so I think I will stick with the prefix.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

Copy link
Copy Markdown
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson mhdawson merged commit 8979194 into main May 30, 2022
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.

3 participants