Remove codepencency as a dependency, use builtin node features instead (fixing #35) #36
Conversation
…ning of winston outside of try/catch
package.json
Outdated
| "json-stringify-safe": "^5.0.1", | ||
| "lodash": "^4.17.15", | ||
| "reconnect-core": "^1.3.0" | ||
| }, | ||
| "peerDependencies": { | ||
| "winston": "^3.2.1", |
There was a problem hiding this comment.
Just curious here but what does the version pin do, in this case? Since it's a peer dependency and as such is not installed by this package, right?
Will it throw up an error now if a customer is using 3.1.x?
There was a problem hiding this comment.
The NPM docs actually seem to indicate that this is the case;
Assuming the host complies with semver, only changes in the host package's major version will break your plugin. Thus, if you've worked with every 1.x version of the host package, use "^1.0" or "1.x" to express this. If you depend on features introduced in 1.5.2, use "^1.5.2".
https://docs.npmjs.com/cli/v7/configuring-npm/package-json#peerdependencies
I think we may need to figure out if we need a specific version or if just 3.x will do 😅
There was a problem hiding this comment.
I believe they will get a warning about an unmet peer dependency rather than an error
There was a problem hiding this comment.
If we can validate it works on all version in 3.x then that is fine to use. The reason this is the base as thats what it was tested with in the devDeps
There was a problem hiding this comment.
I've just checked with version 3.0.0 at least and it works
rjacobs-r7
changed the title
package.json
Outdated
| "json-stringify-safe": "^5.0.1", | ||
| "lodash": "^4.17.15", | ||
| "reconnect-core": "^1.3.0" | ||
| }, | ||
| "peerDependencies": { | ||
| "winston": "^3.x", | ||
| "winston-transport": "^4.3.0" |
There was a problem hiding this comment.
considering we used to require 3.2.1+.... why are we now requiring an even higher version if we can probably work with both?
There was a problem hiding this comment.
Is this referring to #36 (comment) or?
There was a problem hiding this comment.
nope. this sets a minimum version of 4.3.0 for winston-transport, right, whereas we worked with 3.2.1+ before?
There was a problem hiding this comment.
Before it worked with * I believe, but again same as the other version limiting, this is what we have verified works as it's what our devDependencies use
package.json
Outdated
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "r7insight_node", | |||
| "version": "3.2.0", | |||
| "version": "3.2.1", | |||
There was a problem hiding this comment.
Had me confused here :D Version number of r7insight_node and winston lined up
There was a problem hiding this comment.
btw you probably don't want to increase the version number here and in this way, as the git tags will get out of sync.
Also, we're quite possibly breaking compatibility here, so should this be a major?
There was a problem hiding this comment.
This change doesn't break compatibility(if you want to double check yourself, you can install this branch locally using yarn add git+https://github.com/rapid7/r7insight_node.git#fix-module-imports and see how it works with and without winston). I'll revert the package version change
davalentine-r7
force-pushed
the
fix-module-imports
branch
from
0be1b05 to
e6c001d
Compare
Description
This change addresses #35 (comment) which highlights that the
codependencymodule does not work when the library is imported using ES6 import syntax.Testing