Skip to content

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.

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

Merged
jclem 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 1 commit
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Call resolve, then require
  • Loading branch information
@jclem
jclem committed Apr 21, 2021
commit 7e12bd73952c9992711142f5dec2f787345e8c5a
10 changes: 7 additions & 3 deletions dist/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2903,9 +2903,13 @@ const wrapRequire = new Proxy(require, {
return target.apply(thisArg, [moduleID]);
}
catch (err) {
return target.resolve(moduleID, {
paths: eval('module').paths.concat(process.cwd())
});
const modulePath = target.resolve.apply(thisArg, [
moduleID,
{
paths: eval('module').paths.concat(process.cwd())
}
]);
return target.apply(thisArg, [modulePath]);
}
},
get: (target, prop, receiver) => {
Expand Down
11 changes: 8 additions & 3 deletions src/wrap-require.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,14 @@ export const wrapRequire = new Proxy(__non_webpack_require__, {
try {
return target.apply(thisArg, [moduleID])
Copy link
Copy Markdown

@JamesMGreene JamesMGreene Apr 21, 2021

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
Copy Markdown
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.

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
Copy Markdown
Contributor Author

@jclem jclem Apr 21, 2021 •
edited
Loading

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
Copy Markdown

@JamesMGreene JamesMGreene Apr 21, 2021

Choose a reason for hiding this comment

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

Thanks!

} catch (err) {
return target.resolve(moduleID, {
paths: eval('module').paths.concat(process.cwd())
})
const modulePath = target.resolve.apply(thisArg, [
moduleID,
{
paths: eval('module').paths.concat(process.cwd())
Copy link
Copy Markdown

@JamesMGreene JamesMGreene Apr 21, 2021

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
Copy Markdown
Contributor Author

@jclem jclem Apr 21, 2021 •
edited
Loading

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

Expand Down