Skip to content
This repository has been archived by the owner on Jul 5, 2019. It is now read-only.

Typescript LMAO #4

Open
wants to merge 44 commits into
base: master
Choose a base branch
from
Open

Typescript LMAO #4

wants to merge 44 commits into from

Conversation

qm3ster
Copy link

@qm3ster qm3ster commented Jan 13, 2019

The only breaking change is writeStrcut typo (and the corresponding Rsp) being corrected to writeStruct.
Has been tested with the zigbee2mqtt stack.

That's also what I used to profile what of the "enum" package's functionality is needed, not just in own methods.
It wasn't much. That module is essentially being misused here, so I eliminated it. @splitice also did similarly in his forks, replacing it with his "light-enum".
But I believe the "enum" API doesn't represent a useful abstraction here, so I plan to eliminate it entirely, in favor of direct lookups.
The API of the local "enum" replacement in enum.ts (which I want to propagate up the stack) represents this.

I also eliminated the other dependency, "busyman" - a lodash alternative, in favor of native ES5 methods.
There are 0 runtime dependencies now.

Finally, I added decent(?) JSDoc documentation of exported objects and functions.

Also, it's set up to release in the @zigbee npm namespace.

I need this merged and regarded as the status quo so that I can reasonably go up the stack with

  1. zcl-id types being provided
  2. the specific methods on the zcl-id "enums" to replace the overloaded .get

Added `keys` and `values` Maps to Enum for direct lookup.
This enables clearer and more performant code and will make it easier to
see which lookup tables are used / can be eliminated in the whole stack

Added more JSDoc descriptions

Elminated `null` from `cluster_defs.json`
Now the properties are just optional instead

Double quotes (Prettier default)
@splitice
Copy link

So essentially you built enum into the package? Hmm, personally I like the abstraction of a seperate package from a testing point of view.

One of the main reasons we switched to light-enum was the memory usage, light-enum saved us approx 20% of the memory in use on a simple test application. It doesn't rebuild new objects as often for the inverse mappings (remember each lookup works in both directions by default) and makes use of those loaded from source.

@qm3ster
Copy link
Author

qm3ster commented Jan 14, 2019

I currently made the typings not depend on the .jsons at all, they look like

interface CommonJSON {
    profileId: Record<string, number>;
    foundation: Record<string, number>;
    dataType: Record<string, number>;
    status: Record<string, number>;
    clusterId: Record<string, number>;
    haDevId: Record<string, number>;
    [key: string]: Record<string, number>;
}

but this enables @zigbee/zcl-id to be consumed by tsc without --resolveJsonModule option.

Other choices are:

1. Making it depend on the .jsons

As said above, this requires the consumer to have --resolveJsonModule flag or tsconfig.json setting.
This gives us access to the keys, but not the values.
eg, the type of profileId goes

// from
declare const profileId: Record<string, number>
// to
declare const profileId: Record<"HA" | "BA" | "TS" | "HC" | "SE" | "RS" | "LL", number>

2. Converting the .jsons to .tss

This is accomplished by adding export default to the beginning of the file and hitting "save".
This gets us the same result as №1 but without depending on JSON modules.
We can also drop the quotes around keys, and add comments. (someone might be into this)

3. Generating .ts from the .json before compilation

This allows us to do the unthinkable:

// instead of
export default { /* json here */ }
// we can do
export default { /* json here */ } as { /* json here again */ }
// which gets our types all the way to 
declare const profileId: {
    HA: 260;
    BA: 261;
    TS: 263;
    HC: 264;
    SE: 265;
    RS: 266;
    LL: 49246;
};

Do we actually want this? Maybe we do, maybe we don't.
Such enums are sometimes used, and it allows switch exhaustiveness checks, as well as catching typos all the way up the stack - "What are you handling writeStrcut for? We can never decode such a command name.", all without having to exercise that in a test.
As with any "strict" setting, this requires more handling at the edges - "Is this a FoundationCommandName or any string that this function accepts?" but once inside it allows for safe code without guards, once we know we aren't dealing with an arbitrary string.
An alternative to this would be using objects instead of the strings, to have classes like Cluster with fields like attributes, but this is incompatible with the current API, and has (an absolutely negligible) runtime impact.

@qm3ster
Copy link
Author

qm3ster commented Jan 14, 2019

@splitice I probably have a negative impact on memory at the moment, each Enum is 4 maps + and array. (2 maps and the array are references to the same n objects though, and the input object is dropped after constructor)
As I mentioned above the goal of this Enum implementation is not to be anything that will go into public use, but just a stepping stone used to replace what the packages higher in the stack call, until I can switch them over to more pointed lookups by knowing the input type at the call site.

These are the codes described in **11.13.2 OTA Cluster Status Codes** [on page 701](http://www.zigbee.org/~zigbeeor/wp-content/uploads/2014/10/07-5123-06-zigbee-cluster-library-specification.pdf).

Originally added by @splitice in HalleyAssist@34baf8c
This reverts commit 9a27ea8.

We aren't sure what these commands are yet,
but they will be possible to add through the extend hook.
@splitice
Copy link

I'm not keen on the introduction of a typescript step.

An extends method was our original plan, however we had some difficulties making that happen at the time as such a patched fork was the method we went with.

Breaking, typos in identifiers
Old singleton import can be achieved with `require("zcl-id/legacy")`
The source of truth will now be the denormalized deduplicated format
This also includes the responsibilities of `zcl_meta` from `zcl-packet`
Also fixed some missing information discovered
@qm3ster
Copy link
Author

qm3ster commented Jan 18, 2019

@splitice

I'm not keen on the introduction of a typescript step.

Do you mean for the JSONs? Yeah, not doing that currently.
Will maybe end up generating a .d.ts file for better suggestions, but the JSON will remain unmodified in the repo.
However, I am really considering JSON5, because having the numbers as decimals instead of hex sucks. Other repos and the spec PDFs all use hex, so it's bothersome when searching.
Not sure what to do about that.

Additionally, in e25fd3a, I started working on a new source-of-truth format.
For cluster_defs.json it looks like this:

"genIdentify": {
  "id": 3,
  "attrs": {
    "identifyTime": {
      "id": 0,
      "type": "uint16"
    },
    "identifyCommissionState": {
      "id": 1,
      "type": "unknown"
    }
  },
  "cmd": {
    "identify": {
      "id": 0,
      "params": [["identifytime", "uint16"]]
    },
    "identifyQuery": {
      "id": 1,
      "params": []
    },
    "ezModeInvoke": {
      "id": 2,
      "params": [["action", "uint8"]]
    },
    "updateCommissionState": {
      "id": 3,
      "params": [["action", "uint8"], ["commstatemask", "uint8"]]
    },
    "triggerEffect": {
      "id": 64,
      "params": [["effectid", "uint8"], ["effectvariant", "uint8"]]
    }
  },
  "cmdRsp": {
    "identifyQueryRsp": {
      "id": 0,
      "params": [["timeout", "uint16"]]
    }
  }

The param tuples were inspired by your fork, but I don't think having types as numbers is very readable. As you can see, the zcl_meta.json data is all merged into here as well, likewise for functional commands.
I got rid of knownBufLen because its current version is literally just

params.reduce((s, [_, t]) => s + (t === "uint8") || (t === "uint16") * 2)

(I checked)
I also got rid of dir, because all cmd are dir: 0 and all cmdRsp are dir: 1
(I checked)

I am keeping object style for attrs at this time, because there may be other properties we may need to consider.
And yes, I realize [id:number, type:string, writeable:bool, optional: bool] or even [id:number, type:string, options?: {writeable?:bool, optional?: bool}] is an option, just not sure yet.

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.

3 participants