-
-
Notifications
You must be signed in to change notification settings - Fork 11
initial attempt at a typescript definitions file by hand #170
Conversation
There is no change log for this pull request yet. |
I also did some work on this. Maybe this can help:
|
Wow, that's great. I'm still pretty noobish to TS, so that'll take me a little bit to digest. My first question, is there a reason why you've chosen to use enums versus union types? @pwrnrd Thanks! |
@ericblade Yes, I always use enums when there is finite amount of options. Typescript Enums can be used as a type as well as a value. So you can do something like this: enum informalGreeting {
'Hello' = 'Hi',
'Bye' = 'Ciao',
}
function greet(greeting: informalGreeting): void {
console.log(greeting);
}
greet(informalGreeting['Hello']); More specifically, I tend to use sting enums. They cost a little bit more time to write but you get the following benefit
Source: Typescript docs (https://www.typescriptlang.org/docs/handbook/enums.html) |
Hmm. I'm not sure how that's an advantage over using a union type of strings. I'm asking because I am genuinely wanting to understand the differences, improve my knowledge of the domain, and make the best choices i can :) |
No worries, it's a good question. For me the big plus is that enum are values, such that you can do this: Another advantage for me is that, IMO, enums clarify intent better. However, this is very subjective. Example: enum informalGreeting {
'Hello' = 'Hi',
'Bye' = 'Ciao',
}
enum formalGreeting {
'Hello' = 'Goodday Sir/Madam',
'Bye' = 'Goodbye Sir/Madam',
}
function greet(greeting: informalGreeting | formalGreeting): void {
console.log(greeting);
}
greet(informalGreeting['Hello']);
greet(formalGreeting['Hello']);
// VS
type unionGreetings =
'Hi' |
'Ciao' |
'Goodday Sir/Madam' |
'Goodbye Sir/Madam'
}
function unionGreet(greeting: unionGreetings): void {
console.log(greeting);
}
unionGreet('Hi');
unionGreet('Goodday Sir/Madam'); Ofcourse there are also other ways to make this code express it's intent more clearly such as making two functions: "formalGreeting" and "informalGreeting", and many more.... So I think this is just a matter of taste. Other advantages of enums over unions include better typesafety, flag supprot, and namespacing. Although I hardly make use of these advantages and I doubt whether these advantages would come in handy in this package. |
There are probably places where they would, but I have a feeling that something like MWSOrderStatus or MWSRegion or money codes, things like that, where we're just passing an item in that's acceptable to MWS as a string parameter.. I think I would probably stick with a Union string type. Like 767dab5#diff-5a740468657f35267b2cc3f2663588beR57 .. looking at that just for validation/editor completion when passing in one or more statuses to listInboundShipments. I can't presently see, with the knowledge I have right now, why I would use anything more advanced than a Union there. |
All roads lead to Rome |
... i wonder if i started writing tests in TS, if that would make this easier to put together well. As is, i've got a tiny little second project that imports it and allows me to see if the typings are working correctly. |
to chime in here: Using TS a lot, I also tend to use string enums in the cases mentioned above. The main benefit here in contrast to union types is that enums are basically frozen objects, so they still exists after compilation – a union type does not. |
Hi Florian! I do understand that, I'm not sure how that's useful in many cases -- can you give me a relevant example, so I can see where we are coming from with some context that fits the use case? I'm still very noob to TypeScript, but definitely seeing some advantages to it (if nothing else, i'm at least discovering some pretty big holes in my coverage of the input and output parsers in the lib, seeing all the things i haven't got to in the last couple of years, because I didn't need them...), and I want to port my server app to TS, so I started in on this .d.ts... ... another thing I was thinking about last night, since I'm basically re-writing all my tsdoc and validation for the actual Amazon data items into TypeScript, is I wonder if I should put all the items that are direct ports of MWS info into mws-simple (since it's already all typescript), and then export the data types that are mws-advanced specific from here. I'm not sure if that's a possible thing, I haven't tested it yet, just thought about it. |
Hi Eric, first thing that comes to mind: I'm using the api for different marketplaces. So I create enums for marketplaces or report types in my own codebase. Which feels redundant, since it mirrors library code. Thinking about it the other way around: What do union types offer over enums? Also I'm not sure if you're not creating and using some kind of enum-like const objects anyway in your lib code. Regarding the type defs: You can probably use some pre build/publish tooling to share the type definitions and extend the baseline in mws advanced. From a lib consumer perspective I would prefer to have seperate type defs for both packages inside the respective package. |
I agree with you. I think those types should be in MWS Simple. For convenience you can export them again from MWS Advanced. That way, consumers of MWS Advanced don't need to know about MWS Simple. |
Right, that was my first thought upon writing some types that mirror the values of require('mws-advanced').constants . I'm not sure how I would go about writing an enum in a def file, that wouldn't be duplicating the javascript, if I'm not actually prepared to at this moment rebuild any part of the lib into TypeScript. Ideas?
yeah.. i actually wonder if it would be useful to have a separate package with types for all the mws ins and outs.. ultimately, simple wouldn't really be using very much of the data, but could export it for use with that lib.. or it could just be a separate types package? mws-advanced could really use it, though. as well as probably integrating one of the various packages people have made to assist with using TypeScript with incoming JSON data. . . though that might be something more for if/when porting the library rather than just building a definitions file.
Unless there's a way to use the enum type inside the javascript version of the library, they are probably identical to me -- since the javascript library exports several items in it's 'constants' property, which it would still need to do anyway for javascript users. Keep in mind, I'm really noob at typescript, but I don't presently know of any way that I could take advantage of using an enum there. |
You should be able to access constants: MWS_MARKETPLACES, MARKET_CURRENCY, MWS_ENDPOINTS, which are all objects indexed by country code. The list of report types, and other report options that are inside the lib are not exported presently, though I would definitely make a case for exporting that in TypeScript, I'm not sure what would be a good way to expose those to a Javascript user, I guess as an object, similar to a string enum, eh? |
A JS should be able to import an enum as am object, since they are frozen objects. |
... the lib is not built with typescript presently, though. that can / will change, but not in this changeset, which is just experimenting with getting a definitions set that works |
If I can help please let me know. |
well, problem that i'm having right now, is in the test-api.ts file in the commit .. to get intellisense to work correctly, i have to do
but to get the actual test to run, i have to switch it to
which breaks intellisense. i have no idea what i'm doing wrong :-) |
I had a quick look at your ts branch. You have different tsconfig files for your lib and your tests. Is there a specific reason for it? I guess ts config is the reason for the behavior that you described.
I tried and got you test-api.ts running with this. |
@florianbepunkt it's my understanding, since it's importing a node module, that the correct syntax should be The tsconfig.json in the root isn't used, that was from my experiment at auto-generating a .d.ts. Removing it, and the changes you suggested above for the one in test/ don't seem to have any effect -- it continues to work when run, but in vs code, it displays errors. |
@florianbepunkt ok, i guess this works -- adding the esModuleInterop, and switching to "import MWS from" syntax seems to work in both places at the same time. Thanks! |
alright, now that the bare minimum technical issues seem to be conquered, and I can actually see everything working (despite that I went quick-and-dirty on the report functions, rather than making a full type for their inputs, i defined them in the function declaration itself)... time to hash out the bigger details. :-D These are mostly questions that I have in regards to TypeScript in general, and I don't have anyone to bounce these ideas off of professionally, to learn from. So, you all are my teacher :-) 1- would it be better to actually put all the function input parameters in the function declarations, as I did towards the bottom of the file, or is it better to have them as separate type definitions? I haven't consumed a lot of typescript stuff yet, but my thinking is like
If there are no real functional differences, I would actually learn towards putting the function parameters directly in the function declaration, because of the more obvious documentation without having to flip pages 2- I see that both of you tend to use the suggested "I" prefix for Interfaces, but don't prefix anything else. I'm not a big fan, but for something like that, I'll go where custom indicates. That said, I'm not seeing that there's a good reason to use interfaces over type declarations. The current version of the TS Handbook says to always use interfaces, but the new version of the TS Handbook says "choose based on personal preference". Since I'm not anticipating needing to extend anything in here (currently), I'm not seeing a need for that functionality, and if someone else does, you can add to a type declaration of an object form by using &. I'm sure I will have more questions, and I want to get this reasonably close to correct the first time I merge it, so we're not sitting somewhere in not-quite-functional hell while I figure out what works and doesn't work :-D Thanks! (oh, i'm also playing with writing out a declaration for all of the MWS datatypes, straight out of the MWS documentation, to export from mws-simple .. if that works out, it could be also exported from mws-advanced, and used in all the places where we aren't yet providing our own interfaces) |
No functional difference. For readability I'd like to propose to declaring parameters that take objects as an input based on an interface instead of declaring them inline. Primitive types, on the otherhand, may be declared inline. I like to define these interfaces just above the function that uses them. If the interfaces are shared across multiple files I like to create a seperate file (although sometimes I'm a bit to lazy to do that 😇). This is simple a matter of preference though, as stated, there are no functional differences. e.g.
That's weird... sometimes you must use a type instead of an interface...
For me it was just custom. The following eslint rule might be useful: @typescript-eslint/naming-convention to enforce naming. Other ideas for an upgrade (off-topic):
|
That's what I had started to do, and then when I just wanted to get rid of the red error messages in the editor, I just stuck them at the function, just to speed up my entry. Doing it that way does address both. I hadn't tried putting the types inside the class export yet, but i think it'll work?
Sure, major version bump, will make it a bunch easier, clear out some clutter, etc, that was just being held onto for the sake of not having to do new() if you're just doing simple operations. Should file an issue on that
I think the debug options get passed through that, but if you want me to dig through and check, file a question issue :)
I'd love to provide a quick answer, but i'd have to dig through it myself. This is year old code now
sure, they're exported from Javascript, and the current setup for this .d.ts file reflects that by typing them. The 'private' constants should also be exported via typescript also, so they are usable when calling things like getReport |
I fully agree with @pwrnrd. Both points you raised come down to personal preference. I tend to prefer single objects as parameters, even for methods with simple primitive arguments. This way the method input can easily be extended and there is no need for a fixed argument order. One more thing regarding this lib's features. It would be great if the parsing of request responses could be disabled on a case by case basis. For example, I would like to receive reports and store them without parsing the CSV. There are some legal ramblifications involved here: In Germany you would need to store some business data like settlement or shipment reports in its raw form, so parsing is problematic here. |
There's a saveRaw option to callEndpoint that is there to dump raw responses to files (mostly there so when i was developing a parser, i could get to the original data as well) that could be used for that .. i'm not sure if there's access to send that parameter from functions like getReport() though. I want to say if you call callEndpoint yourself, you're not going to go through the parser mechanism like you would if you called getReport(), so you should get the data back as is, but you also have to pass the data in as Amazon expects it, since you don't get the input parameter mangling either. In any case, that sounds like a useful thing, so please file an issue requesting :) |
so, that's not a thing when we're declaring a big class full of functions, as you can't declare a type inside a class :( So, I've pretty much completed what I think should work for it, minus having return definitions for the Reports functions (since many of them could actually return a report, it might take quite some hours of work to come up with anything more specific than "any" .. though several of them don't return reports, i just need to remember what they do return) Sometime over the next several days I hope to find some time to start a new version of my node service in TypeScript, and I want to see how well this works. If it works well, I'll probably merge it, otherwise I'll make some changes, and see what happens. |
- now using lib/index.d.ts so that test/test-api.ts can correctly import it - implemented functions in .d.ts return Promise<whatever>
- discovered missing fields in listOrderItems
- output types need more research, for now set them all as Promise<any>
- fixes so that tests work, *and* the source works correctly in editor
hey @florianbepunkt @pwrnrd any chance you guys could take a look at the current version of this, and let me know if it is any better/worse or same as your local efforts? editor type completion seems to work well, now, in both javascript and typescript projects, but i'm not sure if there's anything more that could be done to improve editor or typescript project compatibility .. at least in this task. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work! Just some small comments from my side.
}; | ||
|
||
// cSpell: disable | ||
type MWS_MARKETPLACES = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice ENUM to export. Whenever a user needs a marketplace ID, the user could simply call this enum, e.g. MWS_MARKETPLACES.CA
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import MWS from '@ericblade/mws-advanced';
MWS.MWS_MARKETPLACES.CA;
should work, it's exported for Javascript consumers as well.
is that good? the "export = " syntax that seems to be required to get it to all function with IDEs with both Javascript and Typescript consumers doesn't allow exporting of anything else .. i'm not sure exactly what that syntax does do, but i know i can't add any other exports to the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pwrnrd what do you think about the existing solution, or do you know of a way to improve upon it, given what we have here?
Looks good to me. I would second pwrnrd suggestion to add the missing default currencies and export marketplaces as enum. |
This is going on 6 weeks now since I've heard anything back on it, I want to move on, so I'm going to merge, and then dig into some of the Issues next time I get a bit of time to devote to this project. I believe that
is the way to get to the exported consts there. If anyone has a better idea for how to make that in TS, please let me know, or submit a pull? Thanks! I really appreciate all of the help given over the time spent figuring this out. |
No description provided.