-
Notifications
You must be signed in to change notification settings - Fork 1k
investigate deps #1348
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
investigate deps #1348
Conversation
… directory removal
…for improved file handling
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #1348 +/- ##
=======================================
Coverage 98.95% 98.95%
=======================================
Files 203 203
Lines 22725 22725
Branches 996 997 +1
=======================================
Hits 22487 22487
Misses 237 237
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
closes #1339 Oh dear. After all this PR reduces the following stats:
but there are still a few things to do: @rollup/plugin-commonjs When using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for this!
package.json
Outdated
"docs:json": "typedoc --options \"./scripts/typedoc.json\" --json \"./docs/tone.json\"", | ||
"remove": "node -e \"fs.rmSync('./build', { recursive: true, force: true })\"", | ||
"build": "npm run increment && npm run remove && npm run ts:build && npm run webpack:build", | ||
"docs": "typedoc --options \"./scripts/typedoc.json\" --json \"./docs/tone.json\"", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we keep this as docs:json, this script is actually referenced in the docs generating script here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad. I didn't know that it's used in the homepage. I'm wondering if it's worth to take a look at the typedoc API or if this should be adjusted in any kind of way?
"build": "npm run increment && rimraf build && npm run ts:build && npm run webpack:build", | ||
"docs": "node scripts/generate_docs.cjs", | ||
"docs:json": "typedoc --options \"./scripts/typedoc.json\" --json \"./docs/tone.json\"", | ||
"remove": "node -e \"fs.rmSync('./build', { recursive: true, force: true })\"", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also out of curiosity, why remove rimraf
? seems like the code was more succinct before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rimraf
pulls in many transitive dependencies. The builtin node module provides the same functionality. The Disadvantage is that it depends on the Nodejs runtime which excludes older Nodejs versions, Bun and Deno users. If it's desired to support them I'd recommend to use premove which has zero dependencies.
I'm uncertain if 'remove' is the best name for this script. One could also name it 'clean' or use it as the npm lifecycle script 'prebuild'.
Less dependencies means a smaller footprint and less maintenance cost
* Adding loop functionality to Sampler (Tonejs#1310) * Addressed nits and fixed linting errors
This reverts commit 5089f42.
thank you!! really appreciate the time you put into making the library simpler and more maintainable 🙏🏻 |
dev
branch. ✅Hey there,
as already mentioned I began to migrate dependencies
fs-extra
,glob
,rimraf
Additionally .mjs and .cjs files were included in the linter config. FYI:
simple-sort-import
does not work with commonJS files (link). This could be a reason to migrate the scripts. I'd be happy to update this PR and implement this change if you're interested.I didn't investigate other dependencies yet but npmgraph provides a good entrypoint to do so.
A few other things that caught my attention but aren't implemented yet:
The npm script(has issue)docs
fails becausescripts/generate_docs.cjs
does not existexecutablePath
to make puppeteer happy and successfully run the tests (I think this has a very low priority)Could you please elaborate whytslib
is included as a dependency? I guess it's because the .ts files are also included via the fieldfiles
inpackage.json
, right?