Support installed npm modules and relative require #135
Conversation
|
Hello from actions/github-script! (3110e8d) |
Co-authored-by: Josh Gross <joshmgross@github.com>
| { | ||
| // Webpack does not have an escape hatch for getting the actual | ||
| // module, other than `eval`. | ||
| paths: eval('module').paths.concat(process.cwd()) |
There was a problem hiding this comment.
Question: Have you tried adding node_modules to the path here? I think that should work, while also preventing accidental resolutions to local modules, i.e. require('hi') => require(process.cwd() + '/hi.js')? 🤔
| paths: eval('module').paths.concat(process.cwd()) | |
| paths: eval('module').paths.concat(path.resolve(process.cwd(), 'node_modules')) |
There was a problem hiding this comment.
I may have mis-tested, but when I tested this, using this method did not result in the ability to require('foo') and have it resolve to ./foo.js. Surprisingly, module.paths.push(process.cwd()) did have this effect, but not this method.
| } | ||
|
|
||
| try { | ||
| return target.apply(thisArg, [moduleID]) |
There was a problem hiding this comment.
Concern: I feel like the order here of the try vs. catch block is backwards.
When using a require('lodash') from my github-script block now, that may end up requiring an incompatible version of the module if it exists as a dependency somewhere "near" to where the github-script code is executed rather than relying on the CWD's package.json file. 😬
There was a problem hiding this comment.
Ah this is a good point. Instead, we should perhaps remove this entire try/catch construct and just do this:
const modulePath = target.resolve.apply(thisArg, [
moduleID,
{
// Webpack does not have an escape hatch for getting the actual
// module, other than `eval`.
paths: [process.cwd(), ...eval('module').paths]
}
])
return target.apply(thisArg, [modulePath])
This adds support for the following:
require('./foo')require('lodash')This is accomplished by wrapping the
requirepassed to the script in a proxy.requirewith a path that starts with'.', we transform that module ID to the result ofpath.join(process.cwd(), moduleID)and then require the absolute path, instead.requirefor some non-relative path and an error is thrown, we catch that error and try again, this time addingprocess.cwd()to the list of paths searched-through.Thanks to @joshmgross and @wraithgar for doing the real work here 😄