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

Reorganize examples into categorized folders #341

Merged
merged 18 commits into from
Nov 3, 2023

Conversation

matthewkeil
Copy link
Contributor

@matthewkeil matthewkeil commented Oct 13, 2023

Reorganized the examples into categorized folders and updated the test_all script for the new directory format.

Here is the layout that was used. Each "category" has several examples and each example can have multiple versions of native integration (nan, node-addon-api, napi):

REPO_ROOT
├── test_all.js
├── package.json
├── README.md
└── src
    ├── 1-getting-started
    │   ├── example1
    │   │   ├── nan
    │   │   ├── node-addon-api
    │   │   └── napi
    │   ├── example2
    │   └── example3
    ├── 2-js-to-native-conversion
    ├── 3-context-awareness
    ├── 4-references-and-handle-scope
    ├── 5-async-work
    ├── 6-threadsafe-function
    ├── 7-events
    └── 8-tooling

The test_all.js file was updated to reflect the new structure and I also massaged the output a bit at the end for a bit more feedback. The file was not originally running the test script in each package, it was just running install. I added a bit of logic to run the test script found in the package.json and then output whether the install works/doesnt, if a test was found/wasnt and if the test ran successfully. I found one that did not run successfully because the script was not setup in the package.json. There is a file in there that looks like a "test" case but its an infinite loop. Will fix that in a subsequent PR.

Updated test_all.js output (after the output that was shown before):

passed: /Users/matthewkeil/Documents/dev/chainsafe/node-addon-examples/threadsafe-function/thread_safe_function_round_trip/napi
passed: /Users/matthewkeil/Documents/dev/chainsafe/node-addon-examples/threadsafe-function/threadsafe-async-iterator/node-addon-api
passed: /Users/matthewkeil/Documents/dev/chainsafe/node-addon-examples/threadsafe-function/typed_threadsafe_function/node-addon-api
no test found:
    /Users/matthewkeil/Documents/dev/chainsafe/node-addon-examples/threadsafe-function/promise-callback-demo/node-addon-api
    /Users/matthewkeil/Documents/dev/chainsafe/node-addon-examples/threadsafe-function/thread_safe_function_counting/node-addon-api
failed install:
    /Users/matthewkeil/Documents/dev/chainsafe/node-addon-examples/general/6_object_wrap/napi
failed tests:
    /Users/matthewkeil/Documents/dev/chainsafe/node-addon-examples/threadsafe-function/thread_safe_function_with_object_wrap/node-addon-api

Questions for how to make this better:

  1. Should the categories and examples be numbered so they show up in a pre-determined order?
  2. Should the categories be placed in a src folder?

@matthewkeil matthewkeil marked this pull request as ready for review October 13, 2023 14:22
@matthewkeil
Copy link
Contributor Author

matthewkeil commented Oct 17, 2023

@mhdawson @legendecas @gabrielschulhof @vmoroz got this updated per the discussion on the meeting last week.

Here is the example of what landing on the repo will look like after this merges:
https://github.com/matthewkeil/node-addon-examples/tree/mkeil/reorganize

@mhdawson
Copy link
Member

@matthewkeil I think its starting to look good, but I think build_with_cmake which is still at the top level should likely be under the src/8-tooling directory?

Copy link
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 with one suggestion/comment

@matthewkeil
Copy link
Contributor Author

Just getting back from vacation. Thanks for pushing that commit @legendecas . If that was the only thing left feel free to merge when you guys are ready @mhdawson

@legendecas legendecas merged commit 7f4a9ed into nodejs:main Nov 3, 2023
16 checks passed
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