paymentsds / mpesa-js-sdk

A JavaScript library aiming to help developers integrating their products with M-Pesa Platform
Apache License 2.0
62 stars 27 forks source link

fix: fix commonjs compatibility #39

Closed thatfiredev closed 4 years ago

thatfiredev commented 4 years ago

~DO NOT MERGE~ Opening this PR for discussion on a few issues that I ran into.

SDK Version: 0.1.0-alpha-4 Node: 10

1. Error importing the SDK

I was testing the SDK with CommonJS. I tried importing it using:

const Client = require("@paymentsds/mpesa").Client;

But it was throwing this error:

internal/modules/cjs/loader.js:638
    throw err;
    ^

Error: Cannot find module '@paymentsds/mpesa'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:636:15)
    at Function.Module._load (internal/modules/cjs/loader.js:562:25)
    at Module.require (internal/modules/cjs/loader.js:692:17)
    at require (internal/modules/cjs/helpers.js:25:18)
    at Object.<anonymous> (/test-paymentsds/index.js:1:16)
    at Module._compile (internal/modules/cjs/loader.js:778:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)
    at Module.load (internal/modules/cjs/loader.js:653:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:593:12)
    at Function.Module._load (internal/modules/cjs/loader.js:585:3)

Adding this line to package.json file seems to have fixed the problem:

https://github.com/paymentsds/mpesa-js-sdk/blob/1f5553bc13efd7f1ce0c1f601cc8900d658403bf/package.json#L6

Not sure if this would be the best fix for the problem.

2. Error when calling receive()

After fixing the error described above, I ran into a second error when calling client.receive():

/node_modules/@paymentsds/mpesa/dist/service.cjs:118
          if (!Object.prototype.hasOwnProperty.call(intent, k) && Object.prototype.hasOwnProperty.call(this.config, correspondences[k])) {
                                                                                                            ^

TypeError: Cannot read property 'config' of undefined
    at map (/node_modules/@paymentsds/mpesa/dist/service.cjs:118:109)
    at Service.fillOptionalProperties (/node_modules/@paymentsds/mpesa/dist/service.cjs:129:18)
    at Service.handleRequest (/node_modules/@paymentsds/mpesa/dist/service.cjs:64:23)
    at Service.handleReceive (/node_modules/@paymentsds/mpesa/dist/service.cjs:49:19)
    at Client.receive (/node_modules/@paymentsds/mpesa/dist/client.cjs:31:27)
    at Object.<anonymous> (/index.js:16:8)
    at Module._compile (internal/modules/cjs/loader.js:778:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)
    at Module.load (internal/modules/cjs/loader.js:653:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:593:12)

Here's my index.js file:

const Client = require("@paymentsds/mpesa").Client;

let client = new Client({
    apiKey: 'MY_API_KEY',             // API Key
    publicKey: 'MY_PUBLIC_KEY',          // Public Key
    serviceProviderCode: '171717' // input_ServiceProviderCode
});

let paymentData = {
    from: 'MY_PHONE_NUMBER',               // input_CustomerMSISDN
    reference: '11114',              // input_ThirdPartyReference
    transation: 'T12344CC',          // input_TransactionReference
    amount: '10'                     // input_Amount
};

client.receive(paymentData).then(function(r) {
    // Handle success scenario
    console.log(r);
}).catch(function(e) {
    // Handle success scenario
    console.error(e);
});

Closes #38 , Closes #42

eltonlaice commented 4 years ago

@edsonmichaque please check it. So, the global variable "this.config" is ignored inside of map method. thanks @rosariopfernandes

` fillOptionalProperties(opcode, intent) { function map(correspondences) { for (const k in correspondences) { if ( !Object.prototype.hasOwnProperty.call(intent, k) && Object.prototype.hasOwnProperty.call(this.config, correspondences[k]) ) { intent[k] = this.config[correspondences[k]]; } }

  return intent;
}

switch (opcode) {
  case C2B_PAYMENT:
  case B2B_PAYMENT:
    return map({ to: "service_provider_code" });

  case B2C_PAYMENT:
    return map({ from: "service_provider_code" });

  case REVERSAL:
    return map({
      initiator_identifier: "initiator_identifier",
      security_credential: "security_credential",
    });

  case QUERY_TRANSACTION_STATUS:
    return map({
      to: "service_provider_code",
    });
}

return intent;

}`

edsonmichaque commented 4 years ago

@rosariopfernandes, thank you for contributting. Putting "main" in package.json is a good path to follow in order to support Node.JS versions below v12, definitely it's a good decision to proceed so.

@eltonlaice and @rosariopfernandes , the second error has to do with the scope of this reference, in ES6+ when using arrow functions it always points to the class instance itself, but it in ES5 it's specific to the scope of the nested function map, since it is not defined there it throws a TypeError. A solution to this is to bind this to self and change every this reference to self inside the nested function map, like this:

 fillOptionalProperties(opcode, intent) {
    const self = this;

    function map(correspondences) {
      for (const k in correspondences) {
        if (
          !Object.prototype.hasOwnProperty.call(intent, k) &&
          Object.prototype.hasOwnProperty.call(self.config, correspondences[k])
        ) {
          intent[k] = self.config[correspondences[k]];
        }
      }

      return intent;
    }

Please, try that and let me know if it works

After fixing the Error 2, run npm run build, to bulid commonjs module to dist

edsonmichaque commented 4 years ago

@rosariopfernandes, we can rename the PR to something broader than "add main field", maybe something like "Fix CommoJS compatibility" since it intents to do it

thatfiredev commented 4 years ago

@edsonmichaque Thanks for the detailed explanation! I did exactly as you instructed and it runs just fine now. I updated the PR with the changes. It's ready to be merged.