Skip to content
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

Support installed npm modules and relative require #135

Merged
merged 25 commits into from Apr 21, 2021
Merged

Support installed npm modules and relative require #135

merged 25 commits into from Apr 21, 2021

Conversation

jclem
Copy link
Contributor

@jclem jclem commented Apr 21, 2021

This adds support for the following:

  • Requiring modules by a path relative to the working directory require('./foo')
  • Requiring npm modules installed in the working directory require('lodash')

This is accomplished by wrapping the require passed to the script in a proxy.

  • When the script calls require with a path that starts with '.', we transform that module ID to the result of path.join(process.cwd(), moduleID) and then require the absolute path, instead.
  • When a script calls require for some non-relative path and an error is thrown, we catch that error and try again, this time adding process.cwd() to the list of paths searched-through.

Thanks to @joshmgross and @wraithgar for doing the real work here 😄

@github-actions
Copy link

github-actions bot commented Apr 21, 2021

Hello from actions/github-script! (3110e8d)

@jclem jclem marked this pull request as ready for review April 21, 2021 20:38
@jclem jclem requested a review from a team as a code owner April 21, 2021 20:38
@jclem jclem changed the title Wrap require to support relative requires Support installed npm modules and relative require Apr 21, 2021
src/wrap-require.ts Outdated Show resolved Hide resolved
@jclem jclem merged commit 95fb649 into main Apr 21, 2021
@jclem jclem deleted the wrap-require branch April 21, 2021 21:50
{
// Webpack does not have an escape hatch for getting the actual
// module, other than `eval`.
paths: eval('module').paths.concat(process.cwd())

Choose a reason for hiding this comment

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

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')? 🤔

Suggested change
paths: eval('module').paths.concat(process.cwd())
paths: eval('module').paths.concat(path.resolve(process.cwd(), 'node_modules'))

Copy link
Contributor Author

@jclem jclem Apr 21, 2021

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 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])

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 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. 😬

Copy link
Contributor Author

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:

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])

Copy link
Contributor Author

@jclem jclem Apr 21, 2021

Choose a reason for hiding this comment

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

Fixing in #136

Choose a reason for hiding this comment

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

Thanks!

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.

None yet

3 participants