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.

Sign up for GitHub

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

Jump to bottom

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

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
ddba1b1
Use non-Webpack-require in evaluated scripts
jclem Apr 21, 2021
f4e5d39
Use a proxy to support relative requires
jclem Apr 21, 2021
e853490
Run build script
jclem Apr 21, 2021
75e3a5b
Move wrapRequire to its own module
jclem Apr 21, 2021
c416f56
Pass nativeRequire, as well
jclem Apr 21, 2021
83c92d6
Rename nativeRequire to __original_require__
jclem Apr 21, 2021
3ca4cd5
Update documentation for relative require support
jclem Apr 21, 2021
c1c139b
Add a catch to support requiring installed modules
jclem Apr 21, 2021
ade5cea
Use concat for paths
jclem Apr 21, 2021
f57c84a
Try global.module
jclem Apr 21, 2021
1f8fc98
Run build
jclem Apr 21, 2021
d37f92f
Test eval
jclem Apr 21, 2021
7e8659d
Run build
jclem Apr 21, 2021
7e12bd7
Call resolve, then require
jclem Apr 21, 2021
b0e12e7
Add Webpack comment vis-a-vis eval
jclem Apr 21, 2021
01f87b6
Remove caveat about node_modules
jclem Apr 21, 2021
3ede589
Add new integration tests
jclem Apr 21, 2021
19e7914
Add integration test running
jclem Apr 21, 2021
4a93ad9
Add actions/checkout
jclem Apr 21, 2021
fb7e8f7
Add npm ci to integration test
jclem Apr 21, 2021
5ee517d
Workflow syntax error
jclem Apr 21, 2021
256da4e
Update src/wrap-require.ts
jclem Apr 21, 2021
c758586
v4.0.0
jclem Apr 21, 2021
b616178
Merge branch 'wrap-require' of https://github.com/actions/github-scri…
jclem Apr 21, 2021
2923e50
Run build script
jclem Apr 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 18 additions & 14 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@ arguments will be provided:
- `core` A reference to the [@actions/core](https://github.com/actions/toolkit/tree/main/packages/core) package
- `glob` A reference to the [@actions/glob](https://github.com/actions/toolkit/tree/main/packages/glob) package
- `io` A reference to the [@actions/io](https://github.com/actions/toolkit/tree/main/packages/io) package
- `require` A proxy wrapper around the normal Node.js `require` to enable
requiring relative paths (relative to the current working directory) and
requiring npm packages installed in the current working directory. If for
some reason you need the non-wrapped `require`, there is an escape hatch
available: `__original_require__` is the original value of `require` without
our wrapping applied.

Since the `script` is just a function body, these values will already be
defined, so you don't have to (see examples below).
Expand Down Expand Up @@ -243,12 +249,10 @@ jobs:
- uses: actions/github-script@v3
with:
script: |
const script = require(`${process.env.GITHUB_WORKSPACE}/path/to/script.js`)
const script = require('./path/to/script.js')
console.log(script({github, context}))
```

_Note that the script path given to `require()` must be an **absolute path** in this case, hence using [`GITHUB_WORKSPACE`](https://docs.github.com/en/actions/configuring-and-managing-workflows/using-environment-variables#default-environment-variables)._

And then export a function from your module:

```javascript
Expand Down Expand Up @@ -282,24 +286,24 @@ jobs:
- uses: actions/checkout@v2
- uses: actions/github-script@v3
env:
SHA: "${{env.parentSHA}}"
SHA: '${{env.parentSHA}}'
with:
script: |
const script = require(`${process.env.GITHUB_WORKSPACE}/path/to/script.js`)
const script = require('./path/to/script.js')
await script({github, context, core})
```

And then export an async function from your module:

```javascript
module.exports = async ({ github, context, core }) => {
const { SHA } = process.env
const commit = await github.repos.getCommit({
owner: context.repo.owner,
repo: context.repo.repo,
ref: `${SHA}`
})
core.exportVariable('author', commit.data.commit.author.email);
module.exports = async ({github, context, core}) => {
const {SHA} = process.env
const commit = await github.repos.getCommit({
owner: context.repo.owner,
repo: context.repo.repo,
ref: `${SHA}`
})
core.exportVariable('author', commit.data.commit.author.email)
}
```

Expand All @@ -324,7 +328,7 @@ jobs:
- uses: actions/github-script@v3
with:
script: |
const execa = require(`${process.env.GITHUB_WORKSPACE}/node_modules/execa`)
const execa = require('execa')

const { stdout } = await execa('echo', ['hello', 'world'])

Expand Down
108 changes: 69 additions & 39 deletions dist/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2883,6 +2883,43 @@ function escapeProperty(s) {

module.exports = require("assert");

/***/ }),

/***/ 366:
/***/ (function(__unusedmodule, __webpack_exports__, __webpack_require__) {

"use strict";
/* harmony export (binding) */ __webpack_require__.d(__webpack_exports__, "wrapRequire", function() { return wrapRequire; });
/* harmony import */ var path__WEBPACK_IMPORTED_MODULE_0__ = __webpack_require__(622);
/* harmony import */ var path__WEBPACK_IMPORTED_MODULE_0___default = /*#__PURE__*/__webpack_require__.n(path__WEBPACK_IMPORTED_MODULE_0__);

const wrapRequire = new Proxy(require, {
apply: (target, thisArg, [moduleID]) => {
if (moduleID.startsWith('.')) {
moduleID = path__WEBPACK_IMPORTED_MODULE_0__.join(process.cwd(), moduleID);
return target.apply(thisArg, [moduleID]);
}
try {
return target.apply(thisArg, [moduleID]);
}
catch (err) {
const modulePath = target.resolve.apply(thisArg, [
moduleID,
{
// Webpack does not have an escape hatch for getting the actual
// module, other than `eval`.
paths: eval('module').paths.concat(process.cwd())
}
]);
return target.apply(thisArg, [modulePath]);
}
},
get: (target, prop, receiver) => {
Reflect.get(target, prop, receiver);
}
});


/***/ }),

/***/ 413:
Expand Down Expand Up @@ -6121,12 +6158,16 @@ function callAsyncFunction(args, source) {
return fn(...Object.values(args));
}

// EXTERNAL MODULE: ./src/wrap-require.ts
var wrap_require = __webpack_require__(366);

// CONCATENATED MODULE: ./src/main.ts






process.on('unhandledRejection', handleError);
main().catch(handleError);
async function main() {
Expand All @@ -6144,7 +6185,15 @@ async function main() {
const github = Object(lib_github.getOctokit)(token, opts);
const script = Object(core.getInput)('script', { required: true });
// Using property/value shorthand on `require` (e.g. `{require}`) causes compilation errors.
const result = await callAsyncFunction({ require: __webpack_require__(875), github, context: lib_github.context, core: core, glob: glob, io: io }, script);
const result = await callAsyncFunction({
require: wrap_require.wrapRequire,
__original_require__: require,
github,
context: lib_github.context,
core: core,
glob: glob,
io: io
}, script);
let encoding = Object(core.getInput)('result-encoding');
encoding = encoding ? encoding : 'json';
let output;
Expand Down Expand Up @@ -6901,25 +6950,6 @@ function expand(str, isTop) {



/***/ }),

/***/ 875:
/***/ (function(module) {

function webpackEmptyContext(req) {
if (typeof req === 'number' && __webpack_require__.m[req])
return __webpack_require__(req);
try { return require(req) }
catch (e) { if (e.code !== 'MODULE_NOT_FOUND') throw e }
var e = new Error("Cannot find module '" + req + "'");
e.code = 'MODULE_NOT_FOUND';
throw e;
}
webpackEmptyContext.keys = function() { return []; };
webpackEmptyContext.resolve = webpackEmptyContext;
module.exports = webpackEmptyContext;
webpackEmptyContext.id = 875;

/***/ }),

/***/ 877:
Expand Down Expand Up @@ -8743,14 +8773,15 @@ function regExpEscape (s) {
/******/ function(__webpack_require__) { // webpackRuntimeModules
/******/ "use strict";
/******/
/******/ /* webpack/runtime/make namespace object */
/******/ /* webpack/runtime/compat get default export */
/******/ !function() {
/******/ // define __esModule on exports
/******/ __webpack_require__.r = function(exports) {
/******/ if(typeof Symbol !== 'undefined' && Symbol.toStringTag) {
/******/ Object.defineProperty(exports, Symbol.toStringTag, { value: 'Module' });
/******/ }
/******/ Object.defineProperty(exports, '__esModule', { value: true });
/******/ // getDefaultExport function for compatibility with non-harmony modules
/******/ __webpack_require__.n = function(module) {
/******/ var getter = module && module.__esModule ?
/******/ function getDefault() { return module['default']; } :
/******/ function getModuleExports() { return module; };
/******/ __webpack_require__.d(getter, 'a', getter);
/******/ return getter;
/******/ };
/******/ }();
/******/
Expand All @@ -8765,6 +8796,17 @@ function regExpEscape (s) {
/******/ };
/******/ }();
/******/
/******/ /* webpack/runtime/make namespace object */
/******/ !function() {
/******/ // define __esModule on exports
/******/ __webpack_require__.r = function(exports) {
/******/ if(typeof Symbol !== 'undefined' && Symbol.toStringTag) {
/******/ Object.defineProperty(exports, Symbol.toStringTag, { value: 'Module' });
/******/ }
/******/ Object.defineProperty(exports, '__esModule', { value: true });
/******/ };
/******/ }();
/******/
/******/ /* webpack/runtime/create fake namespace object */
/******/ !function() {
/******/ // create a fake namespace object
Expand All @@ -8784,17 +8826,5 @@ function regExpEscape (s) {
/******/ };
/******/ }();
/******/
/******/ /* webpack/runtime/compat get default export */
/******/ !function() {
/******/ // getDefaultExport function for compatibility with non-harmony modules
/******/ __webpack_require__.n = function(module) {
/******/ var getter = module && module.__esModule ?
/******/ function getDefault() { return module['default']; } :
/******/ function getModuleExports() { return module; };
/******/ __webpack_require__.d(getter, 'a', getter);
/******/ return getter;
/******/ };
/******/ }();
/******/
/******/ }
);
1 change: 1 addition & 0 deletions src/async-function.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ type AsyncFunctionArguments = {
glob: typeof glob
io: typeof io
require: NodeRequire
__original_require__: NodeRequire
}

export function callAsyncFunction<T>(
Expand Down
11 changes: 10 additions & 1 deletion src/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {context, getOctokit} from '@actions/github'
import * as glob from '@actions/glob'
import * as io from '@actions/io'
import {callAsyncFunction} from './async-function'
import {wrapRequire} from './wrap-require'

process.on('unhandledRejection', handleError)
main().catch(handleError)
Expand All @@ -29,7 +30,15 @@ async function main(): Promise<void> {

// Using property/value shorthand on `require` (e.g. `{require}`) causes compilation errors.
const result = await callAsyncFunction(
{require: require, github, context, core, glob, io},
{
require: wrapRequire,
__original_require__: __non_webpack_require__,
github,
context,
core,
glob,
io
},
script
)

Expand Down
29 changes: 29 additions & 0 deletions src/wrap-require.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import * as path from 'path'

export const wrapRequire = new Proxy(__non_webpack_require__, {
apply: (target, thisArg, [moduleID]) => {
if (moduleID.startsWith('.')) {
moduleID = path.join(process.cwd(), moduleID)
jclem marked this conversation as resolved.
Show resolved Hide resolved
return target.apply(thisArg, [moduleID])
}

try {
return target.apply(thisArg, [moduleID])
Copy link

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 •
edited

Choose a reason for hiding this comment

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

Fixing in #136

Copy link

Choose a reason for hiding this comment

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

Thanks!

} catch (err) {
const modulePath = target.resolve.apply(thisArg, [
moduleID,
{
// Webpack does not have an escape hatch for getting the actual
// module, other than `eval`.
paths: eval('module').paths.concat(process.cwd())
Copy link

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 •
edited

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.

}
])

return target.apply(thisArg, [modulePath])
}
},

get: (target, prop, receiver) => {
Reflect.get(target, prop, receiver)
}
})
1 change: 1 addition & 0 deletions types/non-webpack-require.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
declare const __non_webpack_require__: NodeRequire