Multiple instances of MWSAdvanced overwrite one another
pwrnrd opened this issue · 19 comments
I'm trying to fetch a report in multiple regions. I do this by creating multiple MWS objects. However, the MWS objects seem to overwrite eachother when I create them using an asynchronous function.
Below I wrote some code to give you an example of what I try to do.
const MWS = require('mws-advanced).MWSAdvanced;
function getKeys(region) {
switch (region) {
case "JP":
return {
//some keys for JP
}
default:
return {
//some keys for US
}
}
}
async function createMWSConnection ({accessKeyId, secretAccessKey, merchantId}) {
const mws = new MWS({
accessKeyId,
secretAccessKey,
merchantId,
});
// some async stuff here..
return mws;
};
const requestedReports = ["JP", "NA"].map(regionCode => {
const keys = getKeys(regionCode);
const mws = await createMWSConnection(keys);
return mws.requestReport( .... SOME OPTIONS HERE .... );
}
Does this code make sense? I think the problem could be here:
function requestPromise(requestData, opt = {}) {
return new Promise((resolve, reject) => {
// eslint-disable-line prefer-arrow-callback
const mwsInterface = this && this.mws ? this.mws : mws;
.....
.....
}
}
If I console.log the "this" object, then "this" is undefined. So 'mwsInterface' is set equal to 'mws'. I think that by running my code, the global mws object get overwritten by the second request. Hence, when I try to fetch data from one marketplace I try to do that using the mws object of the second marketplace. What do you think? What else could have caused the behaviour that I'm describing? Is this a bug or is this my own codes wrong doing?
it wouldn't surprise me if there were problems with this invocation. I know what I was trying to do with it, but I am having a bit of a hard time figuring out if it's actually doing what it's supposed to do there.
It'd probably be best to just completely undo all that mess with older style stuff, and just have the module export just a MWSAdvanced class . . if intending to preserve the interface, we can provide a static init() and a non-static init() function. I just tested to make sure that theory works:
> class b {
... static init() { console.warn('* static init'); }
... init() { console.warn('* non static init'); }
... };
undefined
> b.init();
* static init
undefined
> const c = new b();
undefined
> c.init();
* non static init
undefined
>
I'd wholly support doing that, bumping the library version finally, and start following semver. :-D
I am curious what the actual result of your code is there, what problems are coming up?
In my application, at the beginning of each incoming request, I create a new outgoing MWS instance, which I use for several mws service calls. The new instance is to ensure that i'm making the requests using the correct seller's credentials, as what I'm working on has multiple users . . . but does not make requests for multiple marketplaces. It is possible, and I believe that I have tested fairly thoroughly, that multiple users can make multiple requests at the same time, and receive the correct results . . .
Any chance you could write a quick mocha test that illustrates what is broken, so if/when we fix it, we can test? :-D
maybe try the above pull request out? my idea seems to have worked, I seem to be able to export both a class and a static interface, by using a little bit of trickery .. but the trickery is much more straightforward than in the previous version.
i know i need to get the giant test file split up into separate pieces (really, that was my intent from the beginning, i swear lol), but perhaps you can suggest some improvement to the test called 'multiple instances dont become confused at callEndpoint' or additional testing.
Thank you @ericblade ! I'll try the new pull this weekend and hopefully I'm not able to reproduce my error ;) :D! The error was that the MWSSimple object contained references to the JP market, while I was still trying to get a report on the US marketplace. Hence, I was not able to make a correct request (after all, in the JP "region", the US marketplace is not supported).
Seriously, I want to thank you once again... This one had me cracking my brain for a whole (long) day, so I really appreciate that you picked this issue up so fast. It makes me feel supported, thanks!
:-D Well, it is a pretty serious error, and although I'm not experiencing it, I'm only calling CA and US, which share the same endpoint, so maybe that's why I'm not experiencing it. Also when I looked at my code, and couldn't really follow what was going on, I figured I'd better take an hour to see if I could re-write it with that idea that I had. Automated testing saves the day, here, frankly. Without the automated tests, that would've been something I absolutely would not have had any desire to do. :-D
Most of the automated tests use the static-type of call, there's just a cursory couple of tests to make sure that the class form functions..
... worth noting, i haven't tested this with my own application yet. i'm pretty busy with finishing up a contract right now, so i've hardly touched it. i just realized it's entirely possible with the minimal automated testing on the class form, that i haven't covered something that might be broken.
ok, so, i've tested, and it doesn't work (see comments on the pull req).. it's going to be a bit before i can manage to complete this changeover, while most of it is just a simple copy-paste, there's a few rough patches to hammer out. i'll post an updated commit as soon as i can, but i've run out of time tonight.
I'm pulling back a bunch of the separate modules into a single class file, and then implementing the static interface without any trickery, just a separate static function that calls it in a class instance that is allocated if you call the static init() function. So, calling it via static or instance, will both go through the same code path, and not have a slightly weird hack on top of it. It's going to make for one big class, but large chunks of it will be able to be put back into separate files, after . . .
... and I just realized as i'm about a third of the way through doing that, that i could probably keep them all separate as modules, if i pass them a "this" to operate with... argh. i'm about a third of the way through.. but that'll probably be a hell of a lot easier.. and give me a good excuse to clean up all those modules, too.
I think having one class makes sense. Perhaps it's nice to group all functions by API (as defined by the MWS documentation). For exmaple, the MWS documentation describes an "Orders API". We could combine the function related to this API (i.e. get-order, list-order-times, list-orders) in one single file, or perhaps one folder?
Concidering the class, I would be in favor of a big class. I would do this for two reasons:
- it's probably easier for most people to observe the class
- it's probably easier to maintain one class then passing around "this"-objects. If you pass "this" around, then you'll need to lookup where "this" refers to at the place where the function is called/where this is bound to the function. Plus, it's easy to forget a "this" binding when implementing something new.
So I think having one class, might save us time.
I was also thinking about a test this weekend. I think something along these lines might be able to reproduce the error that I encountered, but I'm not entirely sure... Below is the code that I wrote, I will run it on your next push (I haven't run it yet).
it('multiple instances, created asynchronously, connect to the inteded marketplace/region', (done) => {
const createMWSAdvancedAsync = async (initTestParams) => {
const mws = MWS.MWSAdvanced(initTestParams);
// do some async here,
return mws;
};
const allInitParams = [initParams, {...initParams, accessKeyId: "Junk" }];
const allInits = await Promise.all(allInitParams.map(async initParams => {
const MWS = await createMWSAdvancedAsync(initParams);
const MWS.mockRequest = () => {
return Promise.resolve(MWS.init);
};
const init = await MWS.mockRequest();
return init;
}));
expect(allInits[0]).not.to.deep.equal(allInits[1]);
done();
});
i will run that before, and see what comes up.
So, no matter which way it goes, the class layout is basically going to look like
class Mws {
getMarketplaces() { .... }
static getMarketplaces() { ... }
.... repeat for each helper function ...
}
If the bulk of each of those functions is included in the class, it's going to become a very big class. I do appreciate your point about that being easier to handle, in fact, that was the first thing that I thought, and I started re-building it as such. But then when I went to talk about it, I realized that I can better organize all the separate files into their concerns, as you said . . . still have a roadmap inside the class (in fact, can move all of the JSDoc strings into the class) . . but for each helper function, instead of currently doing
const { callEndpoint } = require('./callEndpoint');
module.exports = (params) => callEndpoint(...)(params);
do
module.exports = (api, params) => api.callEndpoint(...)(params);
There's two advantages that I can see right off to this:
1- It's going to be a lot easier to make that change across a bunch of files (just search for everyone who require()s callEndpoint and parseEndpoint, then boilerplate it into the class, and move the JSdoc)
2- For issue #25 it would make for a lot less future setup -- for mocking up server data, the tests can literally just require() each module, pass them a custom API interface that provides a callEndpoint and parseEndpoint that operate on local mock data stores. If I can find the PHP files I had that had the mock data Amazon provided, i can write a test using that in just a minute or two, without having to spin up an express-type server in the mocha test.
The disadvantage to this is it'll still have a pretty monster sized require block at the top of the class, as well as having to boilerplate each function into the class. BUT I'm starting to think that the boilerplate to the class isn't much of a disadvantage -- because the entire public facing API will be all in the one file, with it's JSdoc. It'll still be a single huge file, but I think it'll be pretty obvious how to work with it. Now, let's see what I've learned from my mistakes in the original setup :-)
Okay, I'm curious to see how it works out! Should it be necessary, I can always help with refactoring after we have a working version :).
WOW VSCode's file refactoring stuff is awesome. Moving all of the junk in lib/ and renaming it at the same time was as simple as highlighting each filename in the require(), pressing F2, and entering the new name for the file. It handled moving each file, updating the references inside that file, and changing all the code that required that file, automatically.
well, there it is. some of the mess from older revisions is cleaned up a little bit, but mostly it's just moved. The janky hacks are gone, you can read the class from top to bottom and see the entire public interface. Testing of the higher level functions should be possible by providing a mock callEndpoint that reads datafiles from disk and runs them through xml2json like mws-simple.
Please pick up the issue101 pull req, and run your code that was failing against it, and see what happens?
.. i just realized that this changes it from new MWS.MWSAdvanced() to just new MWS()... i'm not going to worry about changing that.
Yeaaaah VS Code is awesome! Sometimes you might run into a problem where IntelliSense is no longer functioning and variable name changes don't work anymore. I found that restarting VS Code's Typescript server often solves those problems (ctrl+shift+p -> "Typescript: Restart TS server.").
I'm going to the the pull now, and I'll rewind my code, to the version where I had encountered the problem to see whether this solved it :).
I did not check my entire app yet, but I rebuild some of the code where I encountered the error and it seems to work correctly with the update! So, great job!
P.S. I added a pull request to the 'issue 101' branch which contains the test that I wrote. I'm not sure whether it would be able to catch the error that I encountered though, because the test passed immediately...
ever try out the live share stuff?
You mean this stuff? https://github.com/MicrosoftDocs/live-share
yeah, that. i'm really interested in trying it out, but no one i work with actually uses vscode. :-S
@pwrnrd i updated my application to use the latest, and it seems to be working fine. I did have to add another patch for binding this to all the functions in constructor, because i refer to them from different contexts sometimes, but otherwise it all looks like it's working well.