search

hpcc-systems / hpcc-js-wasm

HPCC-Systems Web-Assembly (JavaScript)
https://hpcc-systems.github.io/hpcc-js-wasm/
Apache License 2.0
322 stars 24 forks source link

Extract CLI to dedicated package #229

Closed fregante closed 3 months ago

fregante commented 3 months ago

Publishing an API package and a CLI package separately is rather common:

What do you think about doing the same here? This package has zero dependencies but the CLI (which not everyone needs) has a lot: https://npmgraph.js.org/?q=%40hpcc-js%2Fwasm

@hpcc-js/wasm and @hpcc-js/cli could be published

GordonSmith commented 3 months ago

FWIW the bin file has bundled those dependencies, so I could (should?) move the dependencies into the dev-dependencies section in the package.json?

GordonSmith commented 3 months ago

@fregante having slept on this I think yargs should really be in the dependencies section as its more transparent for consumers of the project (especially from a security reporting standpoint). However I am open to converting this repo to a mono-repo with:

Thoughts?

fregante commented 3 months ago

yargs should really be in the dependencies section

I'd agree with that as long as the npm package does not also bundle it. I haven't looked into your build system but generally anything declared in the dependencies should be left as external/unbundled.

GordonSmith commented 3 months ago

I must confess I am never 100% sure which is "correct convention"... In the case of "yargs" and the "cli", there is no real reason to bundle it (just a habit from my browser focused view of the world).

fregante commented 3 months ago

The logic is that you're asking npm to install the dependency when this package is installed, but then it's completely ignored. Likewise, you're downloading the same dependency twice.

GordonSmith commented 3 months ago

I do understand the mechanics of it! Its more the correct convention - The output of one of my "usual" packages, might bundle the "dependencies", but the "map files" and the "types" might still reference resources from those dependencies...
Also from a security point of view, bundling a third-party library and excluding it from the dependencies feels like its obfuscating its presence (and I know when I review other packages, I start with its package.json dependency list). I am probably over thinking it and should just adopt a clean ESM target as much as possible?

fregante commented 3 months ago

The convention on npm is not to bundle anything. Some package might do so if they optimize for direct usage via CDN like jsdelivr or if there are considerable speed improvements in a hot path module (e.g. a test harness might want to limit the amount of dependencies it ships in order to speed up CI of half the world)

GordonSmith commented 3 months ago

FYI I have published a separate cli to "@hpcc-js/wasm-graphviz-cli", you can see it working with npx -y @hpcc-js/wasm-graphviz-cli -v.
Next I will create a separate "@hpcc-js/wasm-graphviz" package without the cli (and other wasm packages).

GordonSmith commented 3 months ago

FYI @hpcc-js/wasm-graphviz has a release now...

fregante commented 3 months ago

Nice. Are these packages going to be supported? I don't see them in the release script yet: https://github.com/hpcc-systems/hpcc-js-wasm/blob/trunk/.github/workflows/test-and-publish.yml

GordonSmith commented 3 months ago

Yes - I was manually publishing while I was testing...

fregante commented 3 months ago

Next I will create a separate "@hpcc-js/wasm-graphviz" package without the cli (and other wasm packages).

I still see yargs in there:

https://npmgraph.js.org/?q=%40hpcc-js%2Fwasm

GordonSmith commented 3 months ago

Correct - you will want to switch over to use @hpcc-js/wasm-graphviz which is the lean version: https://npmgraph.js.org/?q=%40hpcc-js%2Fwasm-graphviz@1.0.3

fregante commented 3 months ago

Looks great! See https://github.com/npmgraph/npmgraph/pull/290#issuecomment-2284463492