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

feat(attributePicker): support custom attributes (nurikk/zigbee2mqtt-frontend#2001) #2019

Merged
merged 13 commits into from May 24, 2024

Conversation

LaurentChardin
Copy link
Contributor

@LaurentChardin LaurentChardin commented May 10, 2024

This change is linked to Koenkk/zigbee2mqtt#22583 which will update the device definition.
This change is aiming to enable using Custom Clusters attributes in the dev-console, which are not available through ZH.

Updates:

  • Update type definition to introduce Custom Clusters
  • Remove zh and zhc dependencies
  • Duplicate Cluster.ts file as this is becoming static
  • Updating references in components
  • TODO: updating zigbee2mqtt with bridge.ts extension to publish custom clusters as part of bridge/devices topic: (feat) Expose Custom Clusters in MQTT Koenkk/zigbee2mqtt#22583

Notes:

  • @Koenkk with this change, only relevant cluster list are displayed based on the declated clusters in the endpoints. It will not display clusters that cannot be used by the device (in the reporting tab or the dev tab).

@nurikk-sa
Copy link
Contributor

Hi @LaurentChardin! I really appreciate your work and looking forward to merge this pr! 👍

@LaurentChardin LaurentChardin changed the title feat(attributePicker): support custom attributes (nurikk#2001) feat(attributePicker): support custom attributes (nurikk/zigbee2mqtt-frontend#2001) May 13, 2024
@LaurentChardin LaurentChardin marked this pull request as ready for review May 13, 2024 23:49
@LaurentChardin
Copy link
Contributor Author

Updated with the latest change of Koenkk/zigbee2mqtt#22583

@nurikk
Copy link
Owner

nurikk commented May 16, 2024

is it ready to be reviewed?

package.json Show resolved Hide resolved
@nurikk
Copy link
Owner

nurikk commented May 16, 2024

package lock is outdated

@LaurentChardin
Copy link
Contributor Author

LaurentChardin commented May 16, 2024

package lock is outdated

I think i have a different version of pnpm, and since i updated package.json, i didn't check the impact of the build automation. Might have to update .github/workflows/node.js.yml

I am pushing an update with an updated version of pnpm as well as updated version of the github action lib. But i can't test it (or more : i dont know how to :) )

@LaurentChardin
Copy link
Contributor Author

LaurentChardin commented May 17, 2024

@nurikk Is there a way to automate the build test to check the configuration ? or maybe you can have a look ? i am trying to fix it blind :)

Or i should try it on my own repo i guess. Build is working locally of course.

@nurikk
Copy link
Owner

nurikk commented May 17, 2024

@nurikk Is there a way to automate the build test to check the configuration ? or maybe you can have a look ? i am trying to fix it blind :)

Or i should try it on my own repo i guess. Build is working locally of course.

  1. remove version specificator from github action cofigs
  2. "packageManager": "pnpm@9.1.1"
  3. pnpm install

this should fix the problem

@nurikk
Copy link
Owner

nurikk commented May 17, 2024

you should be able to see how your branch is building in Checks tab

@LaurentChardin
Copy link
Contributor Author

you should be able to see how your branch is building in Checks tab

Yes but it seems you need to trigger it to execute it. I think we could have a workflow that is building it automatically on every PR and update of PR, without the need of a manual action or review.

@nurikk
Copy link
Owner

nurikk commented May 17, 2024

I guess github requres approvals when you change actions config. now it should run on every commit.

latest commit failed due to linting issues. can be fixed by pnpm run pretty
https://github.com/nurikk/zigbee2mqtt-frontend/actions/runs/9126981170/job/25096489332?pr=2019

@LaurentChardin
Copy link
Contributor Author

issues

Yes learning how it works the hard way :)

@LaurentChardin
Copy link
Contributor Author

LaurentChardin commented May 17, 2024

The latest commit didnt touch the github action file : still requires your approval to test the build phase.

@Koenkk // @nurikk ( or @nurikk-sa :) ) : can you activate the build test ? The main change has been merged into the z2m dev branch.

LaurentChardin and others added 7 commits May 20, 2024 18:13
- Update type definition to import Custom Clusters
- Remove zh and zhc dependencies
- Duplicate Cluster.ts file as this is becoming static
- Updating references in components
- Supporting new topic `bridge/definitions`
  Introduced in Koenkk/zigbee2mqtt#22583
- Removing previously added files from `zigbee-herdsman`, but keeping
  the enum one
@Koenkk
Copy link
Collaborator

Koenkk commented May 22, 2024

@LaurentChardin Looks like the tests fail, after that I guess this can be merged?

@LaurentChardin
Copy link
Contributor Author

LaurentChardin commented May 22, 2024

@LaurentChardin Looks like the tests fail, after that I guess this can be merged?

The vitest is testing the UI component in a stateless manner. However now we are dependant on the websocket message to retrieve the cluster definition, since we don't have the dependency on ZH anymore : i am not sure how to inject a fake websocket to create the store object in a test case. I am not even sure the test setup is ready for that at all.

Right now, the attribute picker UI component is dependant on the store :

    const { bridgeDefinitions } = store.getState();

There are very little UI test cases defined and none of them are designed to fake a state retrieved from websocket: i believe we need to revamp the whole test suite to enable those kind of tests.

@LaurentChardin
Copy link
Contributor Author

I found a way to test the UI component by injecting the clusters definition through the component attributes.

There might be a better way to test the component with a global state initiated with some server payload, but it does the trick.

@nurikk Happy to do things differently if you have a better approach.

@Koenkk
Copy link
Collaborator

Koenkk commented May 23, 2024

@LaurentChardin can you run npm run pretty? After that this can be merged.

@LaurentChardin
Copy link
Contributor Author

@LaurentChardin can you run npm run pretty? After that this can be merged.

Done

@Koenkk Koenkk merged commit 324538c into nurikk:dev May 24, 2024
2 of 3 checks passed
@Koenkk
Copy link
Collaborator

Koenkk commented May 24, 2024

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

4 participants