MultiMote / niimbluelib

🖨 A library for the communication with NIIMBOT printers
MIT License
6 stars 2 forks source link

Change Event and TypedTargetEvent for React Native compatibility #4

Closed d4n1b closed 1 day ago

d4n1b commented 3 days ago

Hello again,

Thanks again for the hard work on this!

I've been playing around with React Native and, if I'm not mistaken, the Hermes engine doesn't provide global object like Event or EventTarget, which are required for the use of this library.

I forked your repository to try some updates and I could make it work with some changes.

1. TypedEventTarget

The class NiimbotAbstractClient was updated to use EventEmitter from eventemitter3 instead, without losing the types:

import { EventEmitter } from "eventemitter3";
[...]

export abstract class NiimbotAbstractClient extends EventEmitter<ClientEventMap> {

2. Event object

The use of the global object Event (not available in Hermes) was replaced with a custom NiimbotEvent:

import { type ConnectionInfo, type PrinterInfo } from "./client/abstract_client";
[...]

export class NiimbotEvent {
  public type: string;
  public data: any;
  constructor(type: string) {
    this.type = type;
  }
}

export class ConnectEvent extends NiimbotEvent {
  constructor(public info: ConnectionInfo) {
    super("connect");
    this.data = { info };
  }
}

[...]

I was wondering if this is something you would be interested to add to your library to provide a true cross-platform experience. If so, I would be happy to create a PR with the changes - also no offence taken if you prefer to do the changes to what you believe to be the best.

Thanks again! 🙏

MultiMote commented 3 days ago

Sure! While there is no stable version yet, feel free to change anything 😄 I would be happy to accept your PR.

The only thing is, what is the intent of using non-typed data?

export class NiimbotEvent {
  // ...
  public data: any;
}

Can we use typed parameters as before?

d4n1b commented 3 days ago

Brilliant!

I missed the types in the data property. I'll make it a generic so the data can be passed to it.

I'll work on this later today and send the PR over 🦾

There are some other things about other environments being more strict about cyclical imports, but we can discuss if another issue, if you are OK with that?

MultiMote commented 3 days ago

Yes, of course.

d4n1b commented 1 day ago

Hey @MultiMote , sorry for the late reply. I was having some issues with the typing and I think it's already solved.

The PR: https://github.com/MultiMote/niimbluelib/pull/5

I've tested the printing functionality, bluetooth & serial, copying the dist folder from my forked with the eventemitter3 implementation and it seems to work fine in the niimblue repository. The client can now use off/on methods. Alternatively addListener/removeListener.

The types are improved in the client too, so no need to type the events anymore:

image

I also created a PR for the niimblue repository with the changes for the new implementation.

Would you be able to test it locally too before merging? 🙏 - Feel free to give any feedback or any code/implementation preference, if any.

Thanks!

MultiMote commented 1 day ago

Hey. Nice work, I've quickly tested it locally and have no issues with connecting and printing. I will test it more carefully today when I get home.

Thanks for the contribution!