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: add prefix topics #158

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

feat: add prefix topics #158

wants to merge 6 commits into from

Conversation

cjlawson02
Copy link
Owner

@cjlawson02 cjlawson02 commented Nov 3, 2024

Fixes #30

BREAKING CHANGE to add support for prefix topics

Users can subscribe to any path (root or subpath) to receive value updates to their callback

Copy link

codecov bot commented Nov 3, 2024

Codecov Report

Attention: Patch coverage is 95.37037% with 5 lines in your changes missing coverage. Please review.

Project coverage is 87.50%. Comparing base (79c17ad) to head (27205e6).

Files with missing lines Patch % Lines
packages/ntcore-ts-client/src/lib/pubsub/pubsub.ts 85.29% 5 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main     #158    +/-   ##
========================================
  Coverage        ?   87.50%            
========================================
  Files           0       10    +10     
  Lines           0      704   +704     
  Branches        0      137   +137     
========================================
+ Hits            0      616   +616     
- Misses          0       88    +88     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jnallard
Copy link

@cjlawson02
Do you mind publishing a beta package of this change?
It would make it easier for me to test with a real connection the next time I'm at a team meeting

README.md Outdated Show resolved Hide resolved
BREAKING CHANGE: removes immediateNotify from topic subscription
@cjlawson02
Copy link
Owner Author

@cjlawson02 cjlawson02 self-assigned this Nov 14, 2024
@nab138
Copy link

nab138 commented Nov 15, 2024

This is great, however it does not seem to be compatible with the topicsOnly flag, I only want to receive the names/types of all topics, not all of the data from them, is there some way to do this? When I subscribe to a prefix key with topicsOnly: true, the callback never gets called.

@nab138
Copy link

nab138 commented Nov 15, 2024

Also, I noticed that if I am subscribing to a prefix topic of "/" that normal subscriptions stop working (never recieve updates for them). As soon as I comment out allTopics.subscribe(/*...*/) it works as normal.

@cjlawson02
Copy link
Owner Author

Thanks for letting me know, I'll try to get some time this weekend to spin up a simulator to test this

@nab138
Copy link

nab138 commented Nov 15, 2024

Sounds good, thanks for your work on this

@jnallard
Copy link

@cjlawson02 I can confirm that the change did work for me! Thank you! 🥳

This worked for me to listen for all the topics and add all the values into a Redux store:

    const client = NetworkTables.getInstanceByURI('localhost'); // Simulator

    const allTopics = client.createPrefixTopic('/');
    allTopics.subscribe((value, params) => {
        const topicName = params.name;
        const entryExists = !!store.getState().nt.data[topicName];
        if (!entryExists ) {
            store.dispatch(addNTEntry({topicName, type: params.type}))
        }
        store.dispatch(updateNTValue({topicName, value}));
    });

For the record - I tested @nab138's issue with the prefix and normal subscribe and I wasn't able to reproduce the problem:

    const client = NetworkTables.getInstanceByURI('localhost'); // Simulator

    const allTopics = client.createPrefixTopic('/');
    allTopics.subscribe((value, params) => {
        const topicName = params.name;
        if (topicName === '/Shuffleboard/Logging/timeMs') {
            console.log('prefix nt value', value);
        }
    });

    client.createTopic<number>('/Shuffleboard/Logging/timeMs', NetworkTablesTypeInfos.kInteger)
        .subscribe(value => console.log('NOT prefix nt value', value));

image

@nab138
Copy link

nab138 commented Nov 21, 2024

I've done some more experimenting and noticed that the problem is that when I subscribe to the "/" prefix and then attempt a subscription to a double[], I get a bunch of ZodErrors. When I remove it, it works as expected.
image

@cjlawson02
Copy link
Owner Author

@nab138 can you provide a minimal reproducible example please?

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.

Request: Allow listening to all NT changes in a callback method
3 participants