anthonykirby / lora-packet

LoRa radio packet decoder
MIT License
258 stars 83 forks source link

[DRAFT] Upgrade some deps + build ESM release #110

Open jeanmatthieud opened 11 months ago

jeanmatthieud commented 11 months ago

First time that I did that kind of work.

Please try it and fix any issue that you could encounter...

anthonykirby commented 11 months ago

Hi @jeanmatthieud :wave: thank you for the PR; this is the first time that I've looked at ESM, so I've got some stuff to learn!

This PR is also not easy for me to review, because it changes lots of things, even for the legacy:

I need to be sure that none of these break anything for existing users.

I'm not familiar with npx, but I think that npx . does something different than ./node_modules/.bin/eslint

I don't see any test/demo that demonstrates the ESM interface (equivalent to demo1.js)

I probably don't understand, but I don't see why most of these changes are necessary given that it looks possible to create a wrapper (https://nodejs.org/api/packages.html#approach-1-use-an-es-module-wrapper)

jeanmatthieud commented 11 months ago

Hi,

I'm new to that too! I believe that I didn't do any breaking changes; but as stated I'm far from being an expert in this field. The existing code tests passes, and the CLI seems to work. I'm using my fork for a professional project (using ESM), and seems to work fine (for the part I'm using).

Maybe you could apply these changes to a dedicated branch, waiting for other users inputs?

To respond to your questions:

output directory out/ -> cjs/

It should not impact end users, as they should not import the source code of the library.

change of extension e.g. lib/crypto -> lib/crypto.js

Required for TypeScript / ESM. See https://nodejs.org/api/esm.html#esm_terminology and https://github.com/microsoft/TypeScript/issues/40878#issuecomment-702353715 and https://github.com/microsoft/TypeScript/issues/49083#issuecomment-1435399267

multiple dependency upgrades

They were outdated and were causing compilation issue / security warnings.
I didn't changed the aes-cmac version, as it became asynchronous in v2.0.0, and was breaking the compatibility with current usage of lora-packet.

probably unrelated changes in mic.ts; are these required by the linter?

Yes; AesCmac.calculate() should return a Buffer according to the library documentation

I don't know what the option changes in tsconfig.json mean

I followed the recent blog post on everpot, which was stating that using esModuleInterop in a library was forcing the main project to use the same configuration (I didn't checked the source of this statement). If we could avoid using this configuration, was use it? Also cited here: https://github.com/webpack/schema-utils/issues/110

change in build commands & using npx

npx should be install with npm > 5.2 (see this blog post). You can revert this changes, but I believe that it's more readable and easier to maintain.

I'm not familiar with npx, but I think that npx . does something different than ./node_modules/.bin/eslint

You're right it's a typo. I just pushed a commit to fix it.

I don't see any test/demo that demonstrates the ESM interface (equivalent to demo1.js)

I don't really know how to do that and was more focused on my usage 😄

I probably don't understand, but I don't see why most of these changes are necessary given that it looks possible to create a wrapper (https://nodejs.org/api/packages.html#approach-1-use-an-es-module-wrapper)

I didn't see this documentation before. But I believe that you will need a tsc call to build your code; With the solution given on the everpot blog post, only the compilation command changes, and the source code is lightly adapted to be included in an ESM package (like files extensions).