Skip to content

deps: update old and remove unused dependencies #49

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

Merged
merged 1 commit into from
Apr 22, 2020

Conversation

lance
Copy link
Member

@lance lance commented Apr 2, 2020

This commit updates all of the dependencies in package.json to
their most recent versions. It also removes dependencies that were
specified in package.json but not used - or only used in a
require() statement.

These changes have some ripple effects. Istanbul has not been
supported for some time, so it has been replaced with nyc.
The code coverage reporting tool from codacy has been updated
as well. This could not be tested without having the API token.

Finally, the CI job has been modified to run tests on Node.js
versions 10x and 12x. All older versions of Node.js are no longer
maintained.

Fixes: #50

@lance
Copy link
Member Author

lance commented Apr 3, 2020

Not sure why the DCO check didn't re-run. I amended my commit and pushed it with a signoff yesterday as you can see on my fork: lance@201fc8b.

GitHub seems to have caught up with itself.

@lance
Copy link
Member Author

lance commented Apr 16, 2020

@fabiojose just checking in - any thoughts on this?

@fabiojose
Copy link
Contributor

@lance take a look at my reviews. We are dropping the support for 6, 7 and 8, right?

@lance
Copy link
Member Author

lance commented Apr 16, 2020

@fabiojose yes - this commit bumps the tested Node.js versions to 10 and 12. All other versions of Node.js are currently EOL (Node.js 8 may still be in maintenance mode, but it will EOL at the end of April).

I don't see any reviews from you on this. 🤷

@lance lance force-pushed the dependency-updates branch from 201fc8b to 7c1a627 Compare April 16, 2020 19:15
Copy link
Contributor

@fabiojose fabiojose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, my bad! Did not save them. Now they are here.

@@ -1,13 +1,12 @@
language: node_js
node_js:
- 8
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

People with node 8, 7 and 6 will be able to use cloudevent released with version 12?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible, but not guaranteed. I don't think the SDK uses language features that are not available in 6, 7 and 8. But I question the wisdom of even attempting to support a Node.js version that is end of life. Both 6 and 8 are EOL, and 7 was never an LTS version (no odd numbered versions of Node.js should be considered LTS ever).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fabiojose
Copy link
Contributor

@lance Can you add the entry in the CHANGELOG.md and fix that conflicting files?

This commit updates all of the dependencies in package.json to
their most recent versions. It also removes dependencies that were
specified in package.json but not used - or only used in a
`require()` statement.

These changes have some ripple effects. Istanbul has not been
supported for some time, so it has been replaced with nyc.
The code coverage reporting tool from codacy has been updated
as well. This could not be tested without having the API token.

Finally, the CI job has been modified to run tests on Node.js
versions 10x and 12x. All older versions of Node.js are no longer
maintained.

Signed-off-by: Lance Ball <[email protected]>
@lance lance force-pushed the dependency-updates branch from 7c1a627 to aa2cef6 Compare April 20, 2020 18:54
@lance
Copy link
Member Author

lance commented Apr 20, 2020

@fabiojose requested changes have been made.

@fabiojose fabiojose merged commit b03a243 into cloudevents:master Apr 22, 2020
@lance lance deleted the dependency-updates branch April 22, 2020 13:59
@lance
Copy link
Member Author

lance commented Apr 29, 2020

@fabiojose did you see my comment here? The problem occurs during CI and seems to be an issue with the codacy script possibly not handling things well. I don't have credentials to publish codacy results or configure travis, so it's impossible for me to test this.

When I run locally I get this:

**######################################################################## 100.0%
######################################################################## 100.0%
2020-04-29 12:27:25,617 ERROR            com.codacy Invalid configuration: Either a project token or an api token must be provided or available in an environment variable 
**

Note, that in this commit, the code coverage reporting changed here. I noted this in the original PR text:

The code coverage reporting tool from codacy has been updated
as well. This could not be tested without having the API token.

I'll keep poking but I suspect I'll either need credentials for Travis and Codacy or someone who has those credentials will need to fix it.

The failure began with the Merge commit based on this PR. But as you can see, the PR itself passed the Travis build with no problems.

https://travis-ci.org/github/cloudevents/sdk-javascript/builds/677389800

So I'm happy to continue trying to figure out what went wrong, but not sure I can be successful without proper credentials.

@fabiojose
Copy link
Contributor

Seems all is ok.

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.

Dependencies are outdated and contain vulnerabilities
2 participants