Local update

[JeroenVdb]

Referring: JeroenVdb/homebridge-daikin-cloud#77

@Apollon77 I added a method where you update the "local" data only and don't send the change you want to update to Daikin. We could also add an extra parameter to the current setData: onlyLocal: boolean = false but I'm afraid it would complicate the interface of that method.

I also added some tests + test setup (if you want I can also leave this out for now and move it to a separate PR).

Would this be a good approach to do a local update? I think it can be part of this repo?

[jacoscaz]

I think this PR bundles together things that should be evaluated, tested and merged separately and independently of one another.

The changes in package.json appear to imply a switch from CommonJS to ESM, though I also see that the builds now uses tsconfig.build.json which is set to make the TS compiler output CommonJS modules. This is presumably because tsconfig.json, which is set to produce ES modules, is now used by ts-jest.

While these are desirable changes, they are also most definitely major ones that need to be thoroughly tested on their own and should not be bundled together with the change that brings in setLocalData(). Also, as of its more recent versions node.js comes with its own native test runner, which should be more then enough for this specific use case and would not introduce additional dependencies to the project.

Also: of the 11 commits, some date back all the way to 2023. I don't think this is intentional; perhaps this needs to be rebased on the current main branch? If it is intentional, however, this would probably be better off being either rebased to clear up the commit history or --squashed in upon merging.

[Apollon77]

I would agree that we should split that up.

• Adding the setData method and testing could go together
• Changing transpile target to hybrid would be a second thing

[Apollon77]

Ok I would not like to switch to ESM only ... I am ok with a EMS/CJS hybrid package and I did that for thers too but moving to ESM only I do not like tdh

[Apollon77]

why as a public method? Should - when we do that - it shuld not be an internal action whena "set" action was successful , maybe with a special flag to update the internal data? becaise now anyone can manipulate freely the set data .. makes that sense?

[JeroenVdb]

This was the reason for the public method:

We could also add an extra parameter to the current setData: onlyLocal: boolean = false but I'm afraid it would complicate the interface of that method.

But I can also agree with adding an extra parameter to the current setData.

How would you like to name that parameter? Also, I think it should default to the current behaviour (not editing the local data).

async setData(managementPoint: any, dataPoint: any, dataPointPath: any, value: any, ignoreWritableCheck = false, updateLocalData = false) {}

And a follow-up question I then have would be, do we always do the local data update or only when this.#client.requestResource(setPath, setOptions) does not fail? So something like this:

async setData(managementPoint: any, dataPoint: any, dataPointPath: any, value: any, ignoreWritableCheck = false, updateLocalData = false) {
            if (value === undefined) {
            value = dataPointPath;
            dataPointPath = undefined;
        }

        if (!this.managementPoints[managementPoint] || !this.managementPoints[managementPoint][dataPoint] || (dataPointPath && !this.managementPoints[managementPoint][dataPoint][dataPointPath])) {
            throw new Error('Please provide a valid datapoint definition that exists in the data structure');
        }

        const dataPointDef = dataPointPath ? this.managementPoints[managementPoint][dataPoint][dataPointPath] : this.managementPoints[managementPoint][dataPoint];
        this.#validateData(dataPointDef, value, ignoreWritableCheck);

        const setPath =  '/v1/gateway-devices/' + this.getId() + '/management-points/' + managementPoint + '/characteristics/' + dataPoint;
        const setBody = {
            value,
            path: dataPointPath
        };
        const setOptions = {
            method: 'PATCH',
            body: JSON.stringify(setBody),
            headers: {
                'Content-Type': 'application/json'
            }
        } as const;
        
        try {
            const response = await this.#client.requestResource(setPath, setOptions);
            dataPointDef.value = value;
            return response;
        } catch (e) {
            throw new Error('Error setting value: ' + e.message);
        }
    }

[Apollon77]

xI would propose converting the (former) last parameter (ignoreWritableCheck) into an options object and having

boolean (fallback to old) or {ignoreWritableCheck: boolean, updateLocalData: boolean} ... better on the long run.

and yes code like this

[Apollon77]

In this case also the "ignoreWritableCheck" boolen needs to be considered and the value needs to be remapped

[Apollon77]

Sorry that it tool that long for me to check ... reviewed and added my comments mow.

@jacoscaz I honestly squash and merge by default in the meantime - especially for bigger changes as soon as multiple people work on them. Ideally I also rebase anything, but thats just me. Especially for bigger changes with several commits keeping the commit history mostly do not bring too much benefit afterwards but blows up the commit history in the end. With a Squash commit it is more clear what was changed when by whom