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.

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

Trouble importing meshtastic with node #58

Closed
gtnoble opened this issue Aug 27, 2022 · 24 comments
Closed

Trouble importing meshtastic with node #58

gtnoble opened this issue Aug 27, 2022 · 24 comments

Comments

@gtnoble
Copy link

gtnoble commented Aug 27, 2022

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
Copy link
Author

gtnoble commented Aug 27, 2022

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
Copy link
Author

gtnoble commented Aug 27, 2022

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
Copy link
Member

sachaw commented Aug 29, 2022

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
Copy link
Member

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
Copy link
Member

sachaw commented Oct 18, 2022

There's an open PR, I might fork the project and sort this out tonight
timostamm/protobuf-ts#233

@ajmcquilkin
Copy link
Member

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

@sachaw
Copy link
Member

sachaw commented Oct 18, 2022

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
Copy link
Member

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

@ajmcquilkin
Copy link
Member

ajmcquilkin commented Oct 18, 2022

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
Copy link
Member

sachaw commented Oct 18, 2022

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
Copy link
Member

sachaw commented Oct 18, 2022

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
Copy link
Member

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
Copy link
Member

sachaw commented Oct 18, 2022

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
Copy link
Member

sachaw commented Oct 18, 2022

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
Copy link
Member

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
Copy link
Member

sachaw commented Oct 18, 2022

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
Copy link
Member

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

@sachaw
Copy link
Member

sachaw commented Oct 18, 2022

Update on api:

Current:

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

New:

AdminMessage.encodeBinary({
  payloadVariant: {
    field: 'confirmSetRadio',
    value: true
  }
});

@ajmcquilkin
Copy link
Member

Is there a public branch for the potential changes?

@sachaw
Copy link
Member

sachaw commented Oct 21, 2022

Not yet, I'll push some wip changes tonight

@sachaw
Copy link
Member

sachaw commented Oct 31, 2022

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

@ajmcquilkin
Copy link
Member

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
Copy link
Member

sachaw commented Oct 31, 2022

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

@sachaw
Copy link
Member

sachaw commented Jan 19, 2023

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

@sachaw sachaw closed this as completed Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants