Change NotifyOnChangeOnly to accept a list #199

Beastlybear2017 · 2023-11-08T20:13:27Z

I apologise if this is really dumb, I have never attempted to contribute towards a npm package before and have no clue how to / if it is possible to test it from a local file.

Here is my vision of how this would work, although have no clue if it works in practice :)

ghaiklor · 2023-11-09T11:43:08Z

@Beastlybear2017 you can try to write a test, where to check if your solutions works.

Here, for instance, test case that checks if the notifyChange works:

icecast-parser/test/Parser.spec.ts

Lines 57 to 65 in e6ba7a3

    
           it('should properly emit metadata event when metadata has been updated', async () => await new Promise((resolve) => { 
        
             expect.hasAssertions(); 
        
             const radio = new Parser({ autoUpdate: false, notifyOnChangeOnly: true, url: 'https://live.hunter.fm/80s_high' }); 
        
             radio.on('metadata', (metadata) => { 
        
               // @ts-expect-error I want to check that metadata was stored in the private property to later comparsion 
        
               expect(radio.previousMetadata).toStrictEqual(metadata); 
        
               resolve(); 
        
             });

You can add more cases like that where you check scenarios like “what if you provided no keys at all, that way it should notify every time” or “what if I provided the keys that doesn't exist in metadata, it should notify” and so on.

Beastlybear2017 · 2023-11-09T15:55:14Z

I have added the tests (updated yours to use the new system), fixed any issues and commited my changes. All the tests pass / fail when they should
https://github.com/Beastlybear2017/icecast-parser/blob/ccab52975b0fdf5586ff5734e9a171cdcffe779a/test/Parser.spec.ts#L68-L95

Fixed previous issue - Now works as intened

Beastlybear2017 · 2023-11-09T16:28:38Z

Was able to test within my project and fixed an issue that I discorvered. Now works as intened

Updates info for notifyOnChangeOnly

ghaiklor · 2023-11-11T19:04:49Z

Great work!

However, let's keep it backward compatible with older API. Let's leave the possibility to specify the boolean as before.

ghaiklor · 2023-11-11T19:05:56Z

README.md

@@ -47,7 +47,7 @@ You can provide additional parameters to constructor:
 - `userAgent` - by default `icecast-parser`.
 - `keepListen` - by default `false`. If you set to `true`, then response from radio station will not be destroyed and you can pipe it to another streams. E.g. piping it to the `speaker` module.
 - `autoUpdate` - by default `true`. If you set to `false`, then parser will not be listening for recent updates and immediately close the stream. So that, you will get a metadata only once.
- `notifyOnChangeOnly` - by default `false`. If you set both `autoUpdate` and `notifyOnChangeOnly` to `true`, it will keep listening the stream and notifying you about metadata, but it will not notify if metadata did not change from the previous time.
+- `notifyOnChangeOnly` - by default `[]`. If you set both `autoUpdate` to `true` and `notifyOnChangeOnly` to include the a list of metadata keys to listen for (`['StreamTitle', 'StreamUrl']`), it will keep listening the stream and notifying you about metadata, but it will not notify if the selected metadata did not change from the previous time.


By default, let it be false as before for backward compatibility.

ghaiklor · 2023-11-11T19:06:26Z

src/Parser.ts

@@ -9,7 +9,7 @@ export interface ParserOptions {
  errorInterval: number
  keepListen: boolean
  metadataInterval: number
-  notifyOnChangeOnly: boolean
+  notifyOnChangeOnly: string[]


Suggested change

notifyOnChangeOnly: string[]

notifyOnChangeOnly: boolean | string[]

ghaiklor · 2023-11-11T19:06:47Z

src/Parser.ts

@@ -35,7 +35,7 @@ export class Parser extends EventEmitter {
    errorInterval: 10 * 60,
    keepListen: false,
    metadataInterval: 5,
-    notifyOnChangeOnly: false,
+    notifyOnChangeOnly: [],


Suggested change

notifyOnChangeOnly: [],

notifyOnChangeOnly: false,

ghaiklor · 2023-11-11T19:09:06Z

src/Parser.ts

-    for (const [key, value] of metadata.entries()) {
-      if (this.previousMetadata.get(key) !== value) {
-        return true;
+    if (this.options.notifyOnChangeOnly.length > 0) {


Now, here we can check the type of the option. If there is an array, we treat it by your logic but if there is a boolean, nothing changes and works as before.

if (Array.isArray(this.options.notifyOnChangeOnly)) { // your new logic } else if (this.options.notifyOnChangeOnly === true) { // old logic }

ghaiklor · 2023-11-11T19:10:40Z

test/Parser.spec.ts

@@ -57,11 +57,40 @@ describe('parser', () => {
  it('should properly emit metadata event when metadata has been updated', async () => await new Promise((resolve) => {
    expect.hasAssertions();

-    const radio = new Parser({ autoUpdate: false, notifyOnChangeOnly: true, url: 'https://live.hunter.fm/80s_high' });
+    const radio = new Parser({ autoUpdate: false, notifyOnChangeOnly: [], url: 'https://live.hunter.fm/80s_high' });


This change is no longer needed then and this test will check that everything works as before

Beastlybear2017 added 6 commits November 5, 2023 17:40

Update package.json

00fc4f7

Update Parser.ts

f40f258

Update package.json

a7b0ab6

Update Parser.ts

64fbd99

Update package.json

78395a6

Update package.json

403eef9

Beastlybear2017 closed this Nov 8, 2023

Update package.json

ede7812

Beastlybear2017 reopened this Nov 8, 2023

Beastlybear2017 changed the title ~~Change NotifyOnUpdateOnly to accept a list~~ Change NotifyOnChangeOnly to accept a list Nov 8, 2023

Beastlybear2017 added 5 commits November 9, 2023 15:45

Update Parser.spec.ts

e569016

Update Parser.spec.ts

5ddd299

Update Parser.ts

9d1c463

Update Parser.ts

10ce484

Update Parser.spec.ts

ccab529

Update Parser.ts

625dd00

Fixed previous issue - Now works as intened

Update README.md

b90d9bc

Updates info for notifyOnChangeOnly

ghaiklor reviewed Nov 11, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change NotifyOnChangeOnly to accept a list #199

Change NotifyOnChangeOnly to accept a list #199

Beastlybear2017 commented Nov 8, 2023

ghaiklor commented Nov 9, 2023

Beastlybear2017 commented Nov 9, 2023 •

edited

Loading

Beastlybear2017 commented Nov 9, 2023

ghaiklor commented Nov 11, 2023

ghaiklor Nov 11, 2023

ghaiklor Nov 11, 2023

ghaiklor Nov 11, 2023

ghaiklor Nov 11, 2023

ghaiklor Nov 11, 2023

ghaiklor Nov 11, 2023

	notifyOnChangeOnly: string[]
	notifyOnChangeOnly: boolean \| string[]

Change NotifyOnChangeOnly to accept a list #199

Are you sure you want to change the base?

Change NotifyOnChangeOnly to accept a list #199

Conversation

Beastlybear2017 commented Nov 8, 2023

ghaiklor commented Nov 9, 2023

Beastlybear2017 commented Nov 9, 2023 • edited Loading

Beastlybear2017 commented Nov 9, 2023

ghaiklor commented Nov 11, 2023

ghaiklor Nov 11, 2023

Choose a reason for hiding this comment

ghaiklor Nov 11, 2023

Choose a reason for hiding this comment

ghaiklor Nov 11, 2023

Choose a reason for hiding this comment

ghaiklor Nov 11, 2023

Choose a reason for hiding this comment

ghaiklor Nov 11, 2023

Choose a reason for hiding this comment

ghaiklor Nov 11, 2023

Choose a reason for hiding this comment

Beastlybear2017 commented Nov 9, 2023 •

edited

Loading