arfeo
Trying to support non cli
Fixes #18 Fixes #20
Proposed Changes
- Added a new cli-index that is exposed on install into bin folder
- added ability to use require() for nodejs build scripts
- added constants to assist with typing on what type of dependencies to manipulate
- added tests
- slight tweeks to make code more testable
- added a common run function to allow for NodeJS usage
- QOL things such as eslint and editor config to enforce conformation between devs :)
Hey @arfeo,
considering its another working week im only going to get a small amount of time to get the remaining tests done, so figured id get a draft pr going for you to quick scan in case you see anything that makes you too nervous.
I'm still trying to figure out the correct way to test CLI as it seems my argv mock isn't working as intended but i'm sure i will get there soon enough, i have still got the main AddDependencies file to test at the individual function level also.
i tested this in both a cli context and nodejs context before reopening this draft pr in case your wondering if it still works.
Hopefully there isn't much that makes you too nervous 👍
Hey @arfeo,
considering its another working week im only going to get a small amount of time to get the remaining tests done, so figured id get a draft pr going for you to quick scan in case you see anything that makes you too nervous.
I'm still trying to figure out the correct way to test CLI as it seems my argv mock isn't working as intended but i'm sure i will get there soon enough, i have still got the main AddDependencies file to test at the individual function level also.
i tested this in both a cli context and nodejs context before reopening this draft pr in case your wondering
if it still works.Hopefully there isn't much that makes you too nervous 👍
You should give me some time. This PR appears to be quite huge.
hey @arfeo !
finally got some time today to have another crack at the tests. When you get time please give it a review :)
the only set of tests i couldn't seem to get working were the top level cli-index tests, i think this was due to when i try to trigger it with a "require('cli-index')" there's no way for me to tell jest "Okay its done check for expects now please". But this shouldn't be a big deal anyway as im testing the functions themselves also.
so a quick summary
- Added a new cli-index that is exposed on install into bin folder
- added ability to use require() for nodejs build scripts
- added constants to assist with typing on what type of dependencies to manipulate
- added tests
- slight tweeks to make code more testable
- added a common run function to allow for NodeJS usage
- QOL things such as eslint and editor config to enforce conformation between devs :)
Hey! I'd really like to see this feature added, I also need it. The scope of the PR seems quite large, it added ESLint, .editorconfig, tests, etc. I think it should be made into separate PRs to be easier to review and to merge only one thing at a time (because it seems that some tests aren't perfect enough, a lot are commented out, and there are some race conditions according to the comments). Although it might be a little too late 😄
By the way, multiple files don't have a trailing newline, and you should add ESModules exports
Anyway is there still a chance for this to be merged? 😄
ill split them out over the weekend. i agree the pr is fairly large.
as for the tests they are all working i just couldn't figure out how to test it as a cli module
ill look into resolving point too "By the way, multiple files don't have a trailing newline, and you should add ESModules exports" 👍
Hey! I would just like to clarify: when I suggested using ESModule exports, I was talking about allowing people using native Nodejs' modules (with "type": "module" in the package.json) to import it. Using babel is just syntactic sugar, but not actual ESModule exports: it will compile into CommonJS so it doesn't change anything, except for a cleaner code 🙂
To export the library as a native ESModule (in addition to CommonJS and/or babel), you would need to create an index.mjs file exporting the class, etc. Example here, on the discord.js library.
Btw, it might be a bit hard if you've never done TS but I think you should add an index.d.ts (Typescript definitions)... (I'm trying to do it if you want, but I've never done TS definitions so I'm not really sure about what I'm doing 😅)
Also, when using it as a dependency, it will display console messages, right? I think it should not 🤔
Otherwise, I'm really looking forward to these changes to be merged 😄
@noftaly thanks for the input. im a bit of a scrub when it comes to ESModules (as you can tell haha). i got the tests passing by applying the babel then only just got around to testing it with node just now and yeah it didnt play ball.
im going to roll back what i did and just make a .mjs file as your link suggests to give people the option :+1: the one thing im a bit vague on at this moment is how here can call import when they just do module exports in the original index.js, is it magic when "type": "module" is provided that it will just work?
edit: yep its just magic :+1:
ive done a bit of TS (not much) but it is something ive wanted to learn more (code completion is king imo) so if you don't beat me to it ill add that too :)
@noftaly Hey again, i confirmed that the revert + .tsm file works for import with the type: module in package.json :+1: and require still works as it did before
ive also added a script entry for typescript generation for auto .d.ts files ill look into the best way to make the .ts files hopefully tomorrow unless you do it/ find a way :)
Great job!
I would have done the exact same thing but in a single file index.d.ts, under a namespace (as it is done inside most of DefinitelyTyped's packages). But if this works, then you can let it like this 🙂
Hey @arfeo, Hope you have been well!
This Pr is ready for review when you get a chance 👍
The only item left to cross off is how to test the cli context of npm-add. i will have one more try at this later today before throwing in the towel on this
one final followup:
finished the cli context tests so it should be fully ready now :+1:
Actually, I would propose to split this huuuuuge PR to a few smaller ones. The current PR is overloaded with new features which have little to nothing to do with its title.
Let's start with the main idea of this PR... No jest, no eslint, whatever. Not this time.
Hey @arfeo,
Sorry i didnt see the last reply.
I have created 3 prs.
https://github.com/arfeo/npm-add-dependencies/pull/23 - node update
https://github.com/arfeo/npm-add-dependencies/pull/24 - test update
https://github.com/arfeo/npm-add-dependencies/pull/25 - typescript update
Linting - TODO once these are ready