- Notifications
You must be signed in to change notification settings - Fork455
Support installed npm modules and relative require#135
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
github-actionsbot commentedApr 21, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hello from actions/github-script! (3110e8d) |
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Question: Have you tried addingnode_modules to the path here? Ithink 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I may have mis-tested, but when I tested this, using this method didnot result in the ability torequire('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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Concern: I feel like the order here of thetry vs.catch block is backwards.
When using arequire('lodash') from mygithub-script block now, that may end up requiring an incompatible version of the module if it exists as a dependency somewhere "near" to where thegithub-script code is executed rather than relying on the CWD'spackage.json file. 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah this is a good point. Instead, we should perhaps remove this entire try/catch construct and just do this:
constmodulePath=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]}])returntarget.apply(thisArg,[modulePath])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fixing in#136
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks!
Uh oh!
There was an error while loading.Please reload this page.
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 😄