meshtastic / js

JS/TS library for interfacing with Meshtastic devices
https://meshtastic.org
GNU General Public License v3.0
65 stars 33 forks source link

Trouble importing meshtastic with node #58

Closed gtnoble closed 1 year ago

gtnoble commented 1 year ago

I have installed meshtasticjs using npm and there were no errors. When I tried to import in node v16.17.0 using this line: import { IHTTPConnection } from "@meshtastic/meshtasticjs"; I get an error: Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/home/gtnoble/Documents/Projects/meshtastic-test/node_modules/@meshtastic/meshtasticjs/dist/generated/module_config' imported from /home/gtnoble/Documents/Projects/meshtastic-test/node_modules/@meshtastic/meshtasticjs/dist/generated/mesh.js at new NodeError (node:internal/errors:387:5) at finalizeResolution (node:internal/modules/esm/resolve:429:11) at moduleResolve (node:internal/modules/esm/resolve:1006:10) at defaultResolve (node:internal/modules/esm/resolve:1214:11) at nextResolve (node:internal/modules/esm/loader:165:28) at ESMLoader.resolve (node:internal/modules/esm/loader:844:30) at ESMLoader.getModuleJob (node:internal/modules/esm/loader:431:18) at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:76:40) at link (node:internal/modules/esm/module_job:75:36) { code: 'ERR_MODULE_NOT_FOUND' } This could be a mistake on my part, but I thought I would reach out in case it was a deeper problem.

gtnoble commented 1 year ago

I'm not an expert on this, but it looks like the generated files are not adding the .js extension to the relative imports.

gtnoble commented 1 year ago

running node with the option --experimental-specifier-resolution=node seems to be a quick fix for this issue. I suppose in the long run it would be best for the protobuf-ts code generator be modified to generate the proper extensions.

sachaw commented 1 year ago

Yeah, as you've observed, this is the undesirable behavior exhibited by protobuf-ts, I've opened an issue about it before but the developer did not want to invest time into the feature, he was open to PR's though.

ajmcquilkin commented 1 year ago

How does the web client get around this issue when consuming this package? I'm trying to import the JS library in Electron and I'm getting the same error. To my knowledge I'm not able to perform the --experimental-specifier-resolution=node fix.

sachaw commented 1 year ago

There's an open PR, I might fork the project and sort this out tonight https://github.com/timostamm/protobuf-ts/pull/233

ajmcquilkin commented 1 year ago

Sounds good! Happy to help with this if at all possible, this issue is a current blocker for me

sachaw commented 1 year ago

I'm contemplating switching to https://pbkit.dev/ a much more maintainable project, Although I prefer the code generated by protobuf-ts, the project is in shambles and not moving to ESM

ajmcquilkin commented 1 year ago

Interesting, I'll take a look at that. What kind of refactoring would this require?

ajmcquilkin commented 1 year ago

Also looks like this uses Deno. Would there be the possibility for issues running this on Node-based projects?

Edit: the maintainers claim it runs in Node.js or browser here

sachaw commented 1 year ago

Yeah, so the pbkit project is made for deno but: They distribute it for node: https://www.npmjs.com/package/pbkit I would like to make this package Deno native in the future with support for node.

sachaw commented 1 year ago

Issue that I'm talking with the devs about right now is distributing the pb binary, as we currently use protoc that is handled by https://www.npmjs.com/package/node-protoc however I'm not sure of the best way forwards, without making each user manually install this on there system.

ajmcquilkin commented 1 year ago

Interesting, is there no package for the pb binary? Alternatively, is there a situation in which the pb binary is only used in development and doesn't need to be distributed through the package?

sachaw commented 1 year ago

Yeah that's correct, so it's not a massive issue, it's just nice to have it work out of the box for devs

sachaw commented 1 year ago

There will be a few things that are different, string union's instead of enums, imports will be seperated by message not file anymore and a few more

ajmcquilkin commented 1 year ago

Gotcha, so there will have to be some internal rewrites in that case. Would this require API changes and/or a breaking version change?

sachaw commented 1 year ago

The meshtastic.js version will likely be breaking for apps, but the api will be very similar. as for interacting with devices, no change there.

ajmcquilkin commented 1 year ago

Sounds good! Happy to help out with the refactor if this ends up being the best way to move forward

sachaw commented 1 year ago

Update on api:

Current:

Protobuf.AdminMessage.toBinary({
  payloadVariant: {
    confirmSetRadio: true,
      oneofKind: "confirmSetRadio"
    }
});

New:

AdminMessage.encodeBinary({
  payloadVariant: {
    field: 'confirmSetRadio',
    value: true
  }
});
ajmcquilkin commented 1 year ago

Is there a public branch for the potential changes?

sachaw commented 1 year ago

Not yet, I'll push some wip changes tonight

sachaw commented 1 year ago

@ajmcquilkin @gtnoble please check latest release 0.6.113 it should contain a fix for node,

ajmcquilkin commented 1 year ago

Seems to be working for me! Just ran tests in a CRA and a Node.js app and both seem to be working. On a side note, is tslib a required package of this library? If so, is there a reason it isn't included as a dependency? I've had multiple repos error when it isn't installed

sachaw commented 1 year ago

Yeah, need to dig a little deeper into the reasons behind requiring Talib, not sure right now

sachaw commented 1 year ago

Update to this, I've done away with protobuf-ts, now using bufbuild/protobuf-es which supports multiple runtimes. Just sorting out a few last minute bits