ExplorationSystems / opengpio

3 stars 2 forks source link

4 feature request exception handling #8

Closed kueckermann closed 6 months ago

kueckermann commented 6 months ago
kueckermann commented 6 months ago

@Tom-Hirschberger I've added error handling. Could you test this branch? npm install "https://github.com/ExplorationSystems/opengpio.git#4-feature-request-exception-handling" --save

Tom-Hirschberger commented 6 months ago

Hi, and yes for sure i will try to test.

Tried to test but if i install with npm install "https://github.com/ExplorationSystems/opengpio.git#4-feature-request-exception-handling" --save the node_modules/opengpio directory is missing a dist folder and it does not work. Do i have to run a build command, too?

Sorry but i do not have a glue of packaging a NPM

Tom-Hirschberger commented 6 months ago

I installed typescript and tried to run npm build:cpp and npm build:ts

Building C++ works great, TS fails with:

pi@mirror-dev:~/test/node_modules/opengpio $ npm run build:ts

> opengpio@1.0.11 build:ts
> rm -rf dist; tsc

src/classes/Watch.ts:3:30 - error TS2307: Cannot find module 'events' or its corresponding type declarations.

3 import { EventEmitter } from 'events';
                               ~~~~~~~~

src/classes/Watch.ts:19:22 - error TS2339: Property 'emit' does not exist on type 'Watch'.

19                 this.emit('event', value);
                        ~~~~

src/classes/Watch.ts:20:22 - error TS2339: Property 'emit' does not exist on type 'Watch'.

20                 this.emit('change', value);
                        ~~~~

src/classes/Watch.ts:21:22 - error TS2339: Property 'emit' does not exist on type 'Watch'.

21                 this.emit('rise', value);
                        ~~~~

src/classes/Watch.ts:25:22 - error TS2339: Property 'emit' does not exist on type 'Watch'.

25                 this.emit('event', value);
                        ~~~~

src/classes/Watch.ts:26:22 - error TS2339: Property 'emit' does not exist on type 'Watch'.

26                 this.emit('change', value);
                        ~~~~

src/classes/Watch.ts:27:22 - error TS2339: Property 'emit' does not exist on type 'Watch'.

27                 this.emit('fall', value);
                        ~~~~

src/classes/Watch.ts:40:14 - error TS2339: Property 'removeAllListeners' does not exist on type 'Watch'.

40         this.removeAllListeners();
                ~~~~~~~~~~~~~~~~~~

src/lib.ts:1:21 - error TS7016: Could not find a declaration file for module 'bindings'. '/home/pi/test/node_modules/bindings/bindings.js' implicitly has an 'any' type.
  Try `npm i --save-dev @types/bindings` if it exists or add a new declaration (.d.ts) file containing `declare module 'bindings';`

1 import binding from 'bindings';
                      ~~~~~~~~~~

test/watch.ts:4:7 - error TS2339: Property 'on' does not exist on type 'Watch'.

4 watch.on('change', (value) => {
        ~~

test/watch.ts:4:21 - error TS7006: Parameter 'value' implicitly has an 'any' type.

4 watch.on('change', (value) => {
                      ~~~~~

Found 11 errors in 3 files.

Errors  Files
     8  src/classes/Watch.ts:3
     1  src/lib.ts:1
     2  test/watch.ts:4
Tom-Hirschberger commented 6 months ago

Did

npm install "https://github.com/ExplorationSystems/opengpio.git#4-feature-request-exception-handling" --save
cd node_modules/opengpio
npm install --production=false
npm run build:cpp
npm run build:ts

Needed to change the main file in package.json from

"main": "dist/index.js"

to

"main": "dist/src/index.js"

then i am able to test.

I then can catch the error as expected. So fix works but im not sure if you need to change anything to the build process

kueckermann commented 6 months ago

@Tom-Hirschberger I've fixed these issues now. I didn't consider some of the process of npm installing directly from github. Usually I only build the ts before publishing and leave it out of the git so it doesn't clutter up the commits with changes. The ts build should always be done for the published package rather than building on the device like with cpp, since cpp needs native binding for the device. The reason not to allow the device to build the ts files on install is that it requires the ts compiler which is quite a massive library and can severly bloat the package size if it's added as a dependancy.

I've added the dist file back if you wouldn't mind to try again.

Tom-Hirschberger commented 6 months ago

Thank you for the quick response and for implementing the features. I am new to TypeScript so a did not have any clue of how to build. But it worked in the end.

With the new changes everything works as expected.

I noticed that the pins do not get set back to unused if my application is stopped with a SIGTERM. There is a handler in the MagicMirror software i can use to call the stop() function but do you think it is possible to realize some kind of "finally" call of the stop function?

Tom-Hirschberger commented 6 months ago

Sorry, my mistake. I think the close already works. Had the test program running in a second terminal.

kueckermann commented 6 months ago

Ok, let me know if it doesnt auto cleanup but I think it should. There is a stop metod on the pins to do manual cleanup if nessesary,