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

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jeanmatthieud
Copy link

@jeanmatthieud jeanmatthieud commented Oct 18, 2023

First time that I did that kind of work.

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

@jeanmatthieud jeanmatthieud changed the title Upgrade some deps + build ESM release #109 [DRAFT] Upgrade some deps + build ESM release #109 Oct 18, 2023
@jeanmatthieud jeanmatthieud changed the title [DRAFT] Upgrade some deps + build ESM release #109 [DRAFT] Upgrade some deps + build ESM release Oct 18, 2023
@anthonykirby
Copy link
Owner

anthonykirby commented Oct 18, 2023

Hi @jeanmatthieud 👋
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:

  • output directory out/ -> cjs/
  • change of extension e.g. lib/crypto -> lib/crypto.js
  • multiple dependency upgrades
  • probably unrelated changes in mic.ts; are these required by the linter?
  • I don't know what the option changes in tsconfig.json mean
  • change in build commands & using npx

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

jeanmatthieud commented Oct 19, 2023

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 microsoft/TypeScript#40878 (comment)
and microsoft/TypeScript#49083 (comment)

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: webpack/schema-utils#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).

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

Successfully merging this pull request may close these issues.

None yet

2 participants