Skip to content

Conversation

@danny-beton-lrn
Copy link

Add Data API Support to Node.js SDK
This PR adds comprehensive Data API support to the Learnosity Node.js SDK + add the new request header metadata and updated the quickstart demo.

What's New

  • New DataApi class ( lib/DataApi.js) - Routing layer for making authenticated Data API requests
  • Pagination support - requestIter() and resultsIter() async generators for handling multi-page responses
  • Flexible HTTP adapter - Supports custom HTTP clients (defaults to native fetch)
  • Routing metadata headers - Automatic X-Learnosity-Consumer, X-Learnosity-Action, and X-Learnosity-SDK headers
  • Comprehensive tests - 29 passing tests (unit + integration) with 100% coverage
  • Documentation - Updated README, REFERENCE.md, and interactive quickstart demo

@michael-linnane-lrn
Copy link
Contributor

Reviewing now

Copy link
Contributor

@michael-linnane-lrn michael-linnane-lrn left a comment

Choose a reason for hiding this comment

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

Thanks for adding robust tests and docs here @danny-beton-lrn. Just the one comment regarding the codacy warning

}
}
} catch (error) {
demo1_output = [{ error: error.message }];
Copy link
Contributor

Choose a reason for hiding this comment

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

🤓 Nit: This might be better served with a specific error variable rather than making it fit to the demo1_output typing. It would also simplify the conditional logic above by replacing each of the

<% if (demo1_output.length > 0 && demo1_output[0].error) { %>
            <div class="error">Error: <%= demo1_output[0].error %></div>
        <% } else { %>

with something like

<% if (demo1_error) { %>
            <div class="error">Error: <%= demo1_error %></div>
        <% } else { %>
...
<% if (demo2_error) { %>
            <div class="error">Error: <%= demo2_error %></div>
        <% } else { %>

Increases space usage but improves readability

Copy link
Author

Choose a reason for hiding this comment

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

I've made that change, thanks @michael-linnane-lrn 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

👏 Praise: Excellent documentation 🙌

Copy link
Contributor

@michael-linnane-lrn michael-linnane-lrn left a comment

Choose a reason for hiding this comment

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

The only comment left from me is a nit comment. Non blocking so feel free to ignore if you think so

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice work Danny! 👏

I just left a small suggestion.

@michael-linnane-lrn
Copy link
Contributor

LGTM @danny-beton-lrn 🚢

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.

4 participants