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

Dependency update #67

Closed
wants to merge 2 commits into from
Closed

Dependency update #67

wants to merge 2 commits into from

Conversation

Berkmann18
Copy link

I updated some dependencies after getting vulnerability warnings in a project dependent on this one (read more here: Berkmann18@1a7c367) as well as added ISSUE_TEMPLATE (with some core elements) and USE_CASE.md (with one use case so far).
Fixes #65
Closes #64

Fixes

Checklist

  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the [Contribution Guide] and my PR follows them.
  • I updated my branch with the master branch.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation about the functionality in the appropriate .md file
  • I have added inline documentation to the code I modified

Short description of what this PR does:

  • Updated dependencies that were flagged as unmet and/or vulnerable.
  • Added files required to pass 2 tests.

If you have questions, please send an email to Sendgrid, or file a Github Issue in this repository.

License date changed to an up-to-date one
This commit should resolve sendgrid#25 and sendgrid#63.
I updated some packages due to a security vulnerability raised in [sendgrid#65](sendgrid#65) and as a follow up to the PR [sendgrid#64](sendgrid#64) (where I added a basic issue template and one use case).
@Berkmann18
Copy link
Author

The failing tests are docker related (2x, so not my area (yet)) and one regarding ./TROUBLESHOOTING.md which I have no idea how to go about in regards to this package.

@thinkingserious
Copy link
Contributor

Thanks for the update @Berkmann18!

@Berkmann18
Copy link
Author

You're welcome @thinkingserious.

@Berkmann18
Copy link
Author

Is there any plans on merging this PR along with others I've seen?

@thinkingserious
Copy link
Contributor

Hello @Berkmann18,

The plan is to move this repo here, but so far that has not bubbled up on our backlog.

Is that something you would be interested in helping with?

Thanks!

With Best Regards,

Elmer

@Berkmann18
Copy link
Author

@thinkingserious I wouldn't mind helping with that.

@thinkingserious
Copy link
Contributor

Awesome @Berkmann18!

Please let me know how I can help.

I think the first step would be to create a branch and add a nodemailer-sendgrid-transport folder here: https://github.com/sendgrid/sendgrid-nodejs/tree/master/packages

@Berkmann18
Copy link
Author

Okay, I will do that and try to follow the structure that the other directories have in that folder.

@Berkmann18
Copy link
Author

Since the sendgrid package is deprecated and that the sendgrid-transport.js uses it, I was wondering if I should use ../mail/index or ../client/index but both (even when not including index) make npm test fail.

How can I go about resolving that?

@thinkingserious
Copy link
Contributor

Hi @Berkmann18,

Yes, we should update the code to use the latest version of the SDK, which is now @sendgrid/mail.

@Berkmann18
Copy link
Author

Berkmann18 commented Jul 26, 2018

Okay, I managed to move nodemailer-sendgrid-transport to the new location while refactoring it so it would match the structure and style that the other packages use; the problem is the same, whenever I run npm test on the repo it outputs this (using the codebase from this commit:

internal/modules/cjs/loader.js:583
    throw err;
    ^

Error: Cannot find module 'sendgrid'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:581:15)
    at Function.Module._load (internal/modules/cjs/loader.js:507:25)
    at Module.require (internal/modules/cjs/loader.js:637:17)
    at require (internal/modules/cjs/helpers.js:20:18)
    at Object.<anonymous> (/home/maxie/Dropbox/Contribs/sendgrid-nodejs/packages/contact-importer/src/importer.spec.js:9:54)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Object.Module._extensions.(anonymous function) [as .js] (/home/maxie/Dropbox/Contribs/sendgrid-nodejs/node_modules/istanbul/lib/hook.js:107:24)
    at Module.load (internal/modules/cjs/loader.js:599:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
    at Function.Module._load (internal/modules/cjs/loader.js:530:3)
    at Module.require (internal/modules/cjs/loader.js:637:17)
    at require (internal/modules/cjs/helpers.js:20:18)
    at /home/maxie/Dropbox/Contribs/sendgrid-nodejs/node_modules/mocha/lib/mocha.js:250:27
    at Array.forEach (<anonymous>)
    at Mocha.loadFiles (/home/maxie/Dropbox/Contribs/sendgrid-nodejs/node_modules/mocha/lib/mocha.js:247:14)
    at Mocha.run (/home/maxie/Dropbox/Contribs/sendgrid-nodejs/node_modules/mocha/lib/mocha.js:576:10)
    at Object.<anonymous> (/home/maxie/Dropbox/Contribs/sendgrid-nodejs/node_modules/mocha/bin/_mocha:637:18)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Module._extensions..js (internal/modules/cjs/loader.js:700:10)
    at Object.require.extensions.(anonymous function) [as .js] (/home/maxie/Dropbox/Contribs/sendgrid-nodejs/node_modules/babel-register/lib/node.js:152:7)
    at Object.Module._extensions.(anonymous function) [as .js] (/home/maxie/Dropbox/Contribs/sendgrid-nodejs/node_modules/istanbul/lib/hook.js:109:37)
    at Module.load (internal/modules/cjs/loader.js:599:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
    at Function.Module._load (internal/modules/cjs/loader.js:530:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
    at runFn (/home/maxie/Dropbox/Contribs/sendgrid-nodejs/node_modules/istanbul/lib/command/common/run-with-cover.js:122:16)
    at /home/maxie/Dropbox/Contribs/sendgrid-nodejs/node_modules/istanbul/lib/command/common/run-with-cover.js:251:17
    at /home/maxie/Dropbox/Contribs/sendgrid-nodejs/node_modules/istanbul/lib/util/file-matcher.js:68:16
    at /home/maxie/Dropbox/Contribs/sendgrid-nodejs/node_modules/async/lib/async.js:52:16
    at /home/maxie/Dropbox/Contribs/sendgrid-nodejs/node_modules/async/lib/async.js:361:13
    at /home/maxie/Dropbox/Contribs/sendgrid-nodejs/node_modules/async/lib/async.js:52:16
    at done (/home/maxie/Dropbox/Contribs/sendgrid-nodejs/node_modules/async/lib/async.js:246:17)
    at /home/maxie/Dropbox/Contribs/sendgrid-nodejs/node_modules/async/lib/async.js:44:16
    at /home/maxie/Dropbox/Contribs/sendgrid-nodejs/node_modules/async/lib/async.js:358:17
    at LOOP (fs.js:1539:14)
    at process._tickCallback (internal/process/next_tick.js:61:11)

I tried twicking things and tracing where the issue was exactly but I can't seem to find.
I suppose the update you mentioned would impact this so should I wait until it's done then submit a PR to the sendgrid-nodejs repo?

@thinkingserious
Copy link
Contributor

Ah, it looks like the contact-importer package still relies on the old sendgrid package. You may just want to go ahead and install the sendgrid package on your system for testin purposes. Or, you can just submit your PR and I can help fix the tests.

@Berkmann18
Copy link
Author

I tried installing it but it requires to rename the package to something different which then lead to a series of issues around js files in packages with require('@sendgrid/...') and some API changes as well as some apparently missing dependency such as deepmerge in @sendgrid/helpers.
So I'll go ahead and submit the PR.

Berkmann18 added a commit to Berkmann18/sendgrid-nodejs that referenced this pull request Jul 27, 2018
I also refactored some of it such that it contains only the necessary
files, as well as follow the structure that the other sub-packages
follow.

This commit should help with the PRs:
- [NST#67](sendgrid/nodemailer-sendgrid-transport#67)
- [NST#64](sendgrid/nodemailer-sendgrid-transport#64)

And the following issues:
- [NST#65](sendgrid/nodemailer-sendgrid-transport#65)
- [NST#25](sendgrid/nodemailer-sendgrid-transport#25)

The problem I face despit this commit being here is that some
sub-packages require the old `sendgrid` package which works differently
then this one.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lodash in deps have security issue - need to upgrade lodash version
2 participants