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.

Sign up for GitHub

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

Jump to bottom

Typescript types #519

Merged
merged 6 commits into from
Oct 4, 2021
Merged

Typescript types #519

merged 6 commits into from
Oct 4, 2021

Conversation

svrooij
Copy link
Collaborator

@svrooij svrooij commented Oct 3, 2021

I've managed to get started with generating Typescript definition files from existing Javascript code I followed this manual.

This way everybody that likes to use the Sonos library can also enjoy the comfort of automatic documentation.

This PR includes a first version of the type definition. The types are also generated in the prepack stage, so they will never become outdated.

Improving the type definition can be done by adding jsdoc comments to the javascript code.

Check this sample

https://github.com/svrooij/node-sonos/blob/9a87007e3aa81886fca274ed301a64f3bb7ff2ab/lib/services/alarm-clock.service.js#L30-L45

Fixed #512 @realwakils
What do you think @bencevans

@svrooij svrooij requested a review from bencevans October 3, 2021 14:02
@svrooij svrooij linked an issue Oct 3, 2021 that may be closed by this pull request
Add types to node-sonos #512
Open
@svrooij
Copy link
Collaborator Author

svrooij commented Oct 4, 2021

@pascalopitz can you check if you application still works with all these types now included. I've tried to not break anything and just including a first version of the types.

Installing with npm install svrooij/node-sonos#feature/types --save (I guess, never installed a package from git branch).

bencevans
bencevans approved these changes Oct 4, 2021
View reviewed changes
Copy link
Owner

@bencevans bencevans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@svrooij svrooij merged commit bea996e into bencevans:alpha Oct 4, 2021
@github-actions
Copy link

github-actions bot commented Oct 4, 2021

🎉 This PR is included in version 1.15.0-alpha.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@svrooij svrooij deleted the feature/types branch October 4, 2021 10:48
@pascalopitz
Copy link
Contributor

@svrooij Nope, not looking good for me

image

@svrooij
Copy link
Collaborator Author

svrooij commented Oct 4, 2021 •
edited
Loading

@pascalopitz The AsyncDeviceDiscovery is the only thing I've changed. A promise cannot return multiple arguments, so I changed it to return an object with device and model. The model was missing in the async version.

if you change this to:

    async searchForDevices(timeout = 1000) {
        const discovered = await new AsyncDeviceDiscovery().discover({
            timeout,
            port: SONOS_DISCOVERY_PORT,
        });
        await this.connectDevice(discovered.device);
    },

Did this fix your app? Or are you facing tons of troubles?

Related code:

node-sonos/lib/asyncDeviceDiscovery.js

Lines 9 to 21 in bea996e

async discover (options = { timeout: 5000 }) {
return new Promise((resolve, reject) => {
const discovery = DeviceDiscovery(options)
discovery.once('DeviceAvailable', (device, model) => {
discovery.destroy()
resolve({ device, model })
})
discovery.once('timeout', () => {
reject(new Error('No device found'))
})
})
}

@pascalopitz
Copy link
Contributor

Confirmed. I just figured out the same ...

@svrooij
Copy link
Collaborator Author

svrooij commented Oct 4, 2021

Confirmed. I just figured out the same ...

If you're facing any other issues with the new alpha, could you create a new issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bencevans bencevans bencevans approved these changes

Assignees
No one assigned
Labels
hacktoberfest-accepted released on @alpha
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add types to node-sonos
3 participants
@svrooij @pascalopitz @bencevans