Fix/354 #355
Fix/354 #355
Conversation
| } | ||
|
|
||
| function exists (basedir, file) { | ||
| try { | ||
| require.resolve(`${basedir}/${file}`) | ||
| require.resolve(path.join(basedir, file)) |
There was a problem hiding this comment.
I don't think this is necessary since require uses the module syntax and not regular paths, meaning it should always be forward slashes. See nodejs/node#6049 (comment)
| @@ -158,12 +159,12 @@ function getVersion (moduleBaseDir) { | |||
| } | |||
|
|
|||
| function filename (plugin) { | |||
| return [plugin.name, plugin.file].filter(val => val).join('/') | |||
| return path.join.apply(path, [plugin.name, plugin.file].filter(val => val)) | |||
There was a problem hiding this comment.
I would prefer to convert all platform specific separators to forward slashes instead to avoid any incompatibility with require. This would also allow plugins to use simple forward slashes.
| @@ -132,7 +134,7 @@ function getAddress (link) { | |||
| module.exports = [ | |||
| { | |||
| name: 'amqp10', | |||
| file: 'lib/sender_link.js', | |||
| file: path.join('lib', 'sender_link.js'), | |||
There was a problem hiding this comment.
The conversion should be done in the instrumenter to avoid having to do this in plugins. This also applies to all other plugins.
|
@rochdev Yup, all your comments make sense, I'll take another look into it, and try and figure out a better solution. A quick dig last night showed me that something going on in the dd-trace-js/src/instrumenter.js Line 80 in 77ccd7a this._names and in moduleName (and the cause of the issue on windows)
|
|
@rochdev Thanks! I'll close this PR down |
Fixes #354
Ensures we're using native path separator to build paths via the
pathmodule