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

Trying to support non cli #22

Closed
wants to merge 51 commits into from

Conversation

phyzical
Copy link

@phyzical phyzical commented Jun 22, 2020

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 :)

jack added 30 commits June 19, 2020 21:46
…f github.com:phyzical/npm-add-dependencies into Trying-To-Support-non-cli # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
* added eslintrc
* added packagejson changes to support linting
* QOL changes :)
* added the clie index again now that package json isnt borked
…omething

* one slight change to AddDeps to support promise trickle down
* its getting there!
* refactored cli to see if it can use run too
* argv mocked
* figured out how to mock things like process.exit correctly
various fixes to reduce false positives
tried testing cli failed
started the class level tests
@phyzical phyzical marked this pull request as ready for review July 6, 2020 09:45
@phyzical
Copy link
Author

phyzical commented Jul 6, 2020

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 :)

@noftaly
Copy link

noftaly commented Oct 22, 2020

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? 😄

@phyzical
Copy link
Author

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" 👍

@noftaly
Copy link

noftaly commented Oct 27, 2020

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 😄

@phyzical
Copy link
Author

phyzical commented Oct 27, 2020

@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 👍 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 👍

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 :)

…f github.com:phyzical/npm-add-dependencies into Trying-To-Support-non-cli # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
@phyzical
Copy link
Author

@noftaly Hey again, i confirmed that the revert + .tsm file works for import with the type: module in package.json 👍 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 :)

@noftaly
Copy link

noftaly commented Oct 27, 2020

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 🙂

@arfeo
Copy link
Owner

arfeo commented Oct 27, 2020

Hey guys. How're you doing?

@phyzical
Copy link
Author

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

@phyzical
Copy link
Author

one final followup:

finished the cli context tests so it should be fully ready now 👍

@theoludwig
Copy link

@arfeo 👀

@arfeo
Copy link
Owner

arfeo commented Dec 7, 2020

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.

@arfeo
Copy link
Owner

arfeo commented Dec 7, 2020

Let's start with the main idea of this PR... No jest, no eslint, whatever. Not this time.

@phyzical phyzical mentioned this pull request Jan 5, 2021
@phyzical
Copy link
Author

phyzical commented Jan 5, 2021

Hey @arfeo,

Sorry i didnt see the last reply.

I have created 3 prs.

#23 - node update

#24 - test update

#25 - typescript update

Linting - TODO once these are ready

@phyzical phyzical closed this Jul 13, 2023
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.

Feat: Tests feat: Support integration with other Node application
5 participants