Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Typescript definition files for MQ #132

Closed
asselin opened this issue Nov 22, 2021 · 12 comments
Closed

Typescript definition files for MQ #132

asselin opened this issue Nov 22, 2021 · 12 comments

Comments

@asselin
Copy link
Contributor

asselin commented Nov 22, 2021

I've been working on Typescript definition files for this library, and have a draft that is in decent enough shape to start reviewing. Before submitting a PR, I wanted to see if you are interested in adding them to the library. If not, I can go the DefinitelyTyped route instead. If you would consider adding, I'll work on getting corp signoff for the CLA and submit a PR.

Thanks!

@ibmmqmet
Copy link
Collaborator

I'd certainly be interested in them.

One question would be around their maintainability - in particular how easy to keep up to date when new MQI features are released. As an example, the 924 release from last week includes a bunch of new constants MQBNO_, a new structure, and a new field on an existing structure. A lot of the effort to pick those up (the platform-specific cmqc_.js, mqistr.js) is handled automatically by my tooling from the base MQ source. And I've got templates for some other aspects, so I didn't have to do much to add them into the JS wrapper.

@asselin
Copy link
Contributor Author

asselin commented Nov 23, 2021

@ibmmqmet

FYI, with Thanksgiving, it'll be next week before I can get corp signoff to submit the PR.

I won't lie, I did the work by hand, but I feel like at least some of it could be automated. One bit of goodness is that the way I handled MQC values, we don't have to worry about the platform-specific values. The code still accesses MQC.<constant> to get a specific constant value... we're just adding type information to say that MQC.<constant> is of a particular enum type, but I didn't add all the values to new enums.

Let's chat after I submit the PR and see if there's anything that could help automation.

Thanks!

@asselin
Copy link
Contributor Author

asselin commented Dec 3, 2021

FYI, corp approval is in process. Hopefully early next week I'll be able to submit the PR.

@asselin asselin mentioned this issue Dec 6, 2021
4 tasks
@asselin
Copy link
Contributor Author

asselin commented Dec 6, 2021

@ibmmqmet PR is posted to start discussing. Two things in particular:

  • enum values. I added types for the existing JS API, where the user references it as MQC.<constant>. It's possible to fill out the enums at the top of the mqc.d.ts with the actual values. This would make e.g. Intellisense more useful by helping to show what values are possible, however it'd be different than the JS API. It is possible to do the same thing for the JS API too (so in addition to the 1 big MQC enum, there can be enums for each group of constants). I'm interested in what your opinion is for this.
  • I added some files around eslint, etc. These aren't strictly necessary, and could e.g. be moved into the TS samples directory to be more "scoped".

Also, FYI, I have about 5 more samples to convert from JS to TS.

Looking forward to hearing your feedback. Thanks!

@ibmmqmet
Copy link
Collaborator

ibmmqmet commented Dec 9, 2021

Thanks for this. It'll take me a few days to look closely at it, but my first thought is around the enums. None of the other MQ APIs work that way - there's no explicit tying of a prefix to a particular field in a structure for example. And the prefixes (first part of the definition) are not always unique. For example, MQTA_PUB_* and MQTA_SUB_* are overlapping sets of constants. There are also duplicates in some views of the sets - MQPL_AIX is the same value as MQPL_UNIX - which might be a problem with some uses of enums.

I can see benefit to having separate sets, so I'll see how well they work here.

@asselin
Copy link
Contributor Author

asselin commented Dec 9, 2021

@ibmmqmet I added the enums so that typescript could detect invalid usage of the constants. As implemented right now, it doesn't change the code that the user writes (see the TS examples). I'll admit that I'm an MQ novice, so I did the best I could with defining them. I'll fix any bugs that you spot.

The other thing I was saying about the enums is debating whether to actually fill the constant values into enum declarations. For example:

  const enum MQC_MQGMO {
    ACCEPT_TRUNCATED_MSG = 64,
    ALL_MSGS_AVAILABLE = 131072,
    ALL_SEGMENTS_AVAILABLE = 262144,
    // etc.
  }

This would let you write code that references the values in the enum, for example:

  gmo.Options =
    mq.MQC_MQGMO.NO_SYNCPOINT |
    mq.MQC_MQGMO.NO_WAIT |
    mq.MQC_MQGMO.CONVERT |
    mq.MQC_MQGMO.FAIL_IF_QUIESCING;

with the benefit of getting better Intellisense from VSCode:

image

Of course, we could adjust prefixes and such if needed.

@ibmmqmet
Copy link
Collaborator

I've finally had some time to look at this in more detail and do some testing (log4j stuff and holidays have got in the way). It's rather impressive.

The biggest problem I had with it was the use of reference types= in the main index file. When I was trying to run some test programs either from inside the samples/typescript directory or in a separate tree that used the local directory, then although the index file was being read, none of the included files seemed to be pulled in. And that of course led to loads of errors about missing type info. Changing to reference path= made it all work fine. I don't know if that was due to incorrect configuration (eg tsconfig.json) or different levels of compiler. I was doing it all directly from command line, not using an IDE like VSCode.

I did find a few minor compile errors in the structure definitions, but nothing too bad. And I do now have a script that generates mqc.d.ts reasonably automatically so it should be easier to maintain when new MQI constants get defined - I can run that at the same place as when I generate the mqidefs files. Where I also get warned if new fields are added to structures or new functions need to be handled.

@ibmmqmet
Copy link
Collaborator

ibmmqmet commented Jan 3, 2022

I found something else I didn't quite understand was the handling of bitfield values such as the MQGMO.Options field. You have declared that with

class MQGMO {
    Options: MQC_MQGMO;

If I deliberately use an incorrect single value, eg gmo.Options = MQC.MQPMO_SYNCPOINT then it is flagged as an error. But if I do gmo.Options |= MQC.MQPMO_SYNCPOINT to add the flag to any existing ones, then it is not an error. The field is not really an MQC_MQGMO type, but an aggregration of several values from that enum.

This is certainly handled no worse than in any of the other MQI language bindings, where the field is simply an MQLONG integer. But I wondered whether the type information in the declaration was providing any compilation value (even if it's a hint for IDEs).

@asselin
Copy link
Contributor Author

asselin commented Jan 3, 2022

@ibmmqmet Thanks, and happy new year!

You're right about the path= vs types=, but the typescript handbook also says to avoid path=. I'll play with it some more and see what I can come up with.

Interesting find with the bit values. I'll research this-- with bitwise OR being seldom used in JS, I wouldn't be surprised if this is a limitation of the compiler's type checking, but again, I'll play with it and see.

Have you given any thought to the question I posted on Dec 9 re: how we define the enums?

As far as what has to be completed in order to merge the PR, I think there's ~5 examples that need a TS version, and I'd like to look at making the config files local to the TS directory. Is there anything you have on your punchlist that you'd like to see fixed/changed before merging?

Thanks!

@ibmmqmet
Copy link
Collaborator

ibmmqmet commented Jan 4, 2022

I don't like the idea of explicitly filling in the enums in that way. Partly because it makes the code look different from other languages, which makes it irritating when porting code or trying to use examples from other places. Partly because the enum names are not always as obvious as you might think, as I've mentioned before. Making people use the full MQI constant definition will hopefully reduce confusion if they try to use some of those ranges.

What I'm thinking of doing is to merge the code in its current state, where I've made a few fixes and using the script-generated versions of a couple of the definition files. And include a note in the README about this being phase 1 and subject to change (the path= and dealing with the bitfields questions for example might cause a change).

Then additional samples or instructions or config files can be added in a separate PR. That way, it'll get the basic support out for other people to try in a more natural way by referencing it from the npm repository rather than having to mess with git clones and branches.

@ibmmqmet
Copy link
Collaborator

ibmmqmet commented Jan 4, 2022

To get better assistance on the Options bitfields, one idea I have would be to redefine the classes eg

class MQPMO { Options: number | MQC_MQPMO[];

and then have that ripple through into the core JS functions, eg copyPMOtoC would have if isArray(jspmo.Options) {mqpmo.Options = all elements combined} but that's a bigger change.

Again, that's not done in any of the other MQI bindings. So I think just sticking to number to start with is good enough and is not actively incorrect.

@ibmmqmet
Copy link
Collaborator

Thanks for the work on this. It's now been merged and published as part of 0.9.20.

You'll probably spot a few changes from the original submission partly where I've used some scripts to auto-gen pieces, and some alterations in the datatypes.

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

No branches or pull requests

2 participants