Skip to content
This repository has been archived by the owner on Jun 28, 2023. It is now read-only.

fix to use NPM package #182

Closed
wants to merge 1 commit into from

Conversation

v-geberr
Copy link

Fixes #
#124

Proposed Changes

There are two issues:

  1. sample code used from NPM package page throws error of Fetch not found. Sticking it in the global, as it is currently done is not working for the NPM package usage. This fix adds the dependency to the top of serviceBase.js.

  2. Calling via npm package instead of cli, there is no way to pass in LUIS config values because the API begins at /api/lib and the params passed into a method are not overriding the serviceBase.config values. This fix allows the method call with params to fix that. I could see an argument for beginning the package at /api instead but the config settings at the top of serviceBase is not clear what the code is attempting to do.

correct example call:

var LUIS = require('luis-apis').Apps;
var config = require('dotenv').config;
config();
var getApps = async() =>{
    try{

        var ApiApps = new LUIS.Apps();
        
        var response = await ApiApps.Get({
            appId: process.env.LUIS_APP_ID, 
            versionId:process.env.LUIS_VERSION_ID, 
            authoringKey: process.env.LUIS_AUTHORING_KEY,
            endpointBasePath: process.env.LUIS_ENDPOINT_BASE_PATH
        });

        var body = await response.json();

        console.log(body);

    }catch(err){
        console.log(err);
    }
}

getApps();

Testing

You don't have any npm-package based tests so I didn't want to assume anything here. I'm happy to write mocha tests though.

Docs

The docs for the NPM package are light. I would love to be able to use this package when I write the
samples for the LUIS docs. If you would like some help with these docs, let me know.

@v-geberr
Copy link
Author

@tomlm @vishwacsena Please review.

@justinwilaby
Copy link
Contributor

This PR will pull in the node-fetch dependency for browser clients when we should use the native fetch api instead.

When used as a node library, it's probably best to supply it as a global somewhere. e.g.

global.fetch = require('node-fetch');

@justinwilaby justinwilaby self-requested a review May 16, 2018 13:58
Copy link
Contributor

@justinwilaby justinwilaby left a comment

Choose a reason for hiding this comment

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

This is best accomplished by supplying node-fetch as a global somewhere in the node program but outside this library. The documents could be updated to reflect this.
global.fetch = require('node-fetch');

@v-geberr v-geberr closed this Jul 5, 2018
JuanAr pushed a commit to southworks/botbuilder-tools that referenced this pull request Aug 23, 2018
* Parameter Checks and Documentation

* Functional Tests

* Refactor TableStorage
- Store object properties into multiple columns
- Read EDM information and re-create JS object with proper types

* Docs update

* Avoid storing eTag as row column
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants