GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Support installed npm modules and relative require by jclem · Pull Request #135 · actions/github-script

Conversation

@jclem

@jclem jclem commented Apr 21, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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 😄

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
kittiphophum18 reacted with thumbs up emoji JamesMGreene and joshmgross reacted with hooray emoji
All reactions
  • 👍 1 reaction
  • 🎉 2 reactions
jclem added 2 commits April 21, 2021 16:16
@jclem
Use non-Webpack-require in evaluated scripts
ddba1b1
@jclem
Use a proxy to support relative requires
f4e5d39
@github-actions

github-actions Bot commented Apr 21, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jclem jclem mentioned this pull request Apr 21, 2021
Use non-Webpack-require in evaluated scripts #134
Closed
jclem added 2 commits April 21, 2021 16:35
@jclem
Run build script
e853490
@jclem
Move wrapRequire to its own module
75e3a5b
@jclem
jclem marked this pull request as ready for review April 21, 2021 20:38
@jclem
jclem requested a review from a team April 21, 2021 20:38
jclem added 11 commits April 21, 2021 16:40
@jclem
Pass nativeRequire, as well
c416f56
@jclem
Rename nativeRequire to __original_require__
83c92d6
@jclem
Update documentation for relative require support
3ca4cd5
@jclem
Add a catch to support requiring installed modules
c1c139b
@jclem
Use concat for paths
ade5cea
@jclem
Try global.module
f57c84a
@jclem
Run build
1f8fc98
@jclem
Test eval
d37f92f
@jclem
Run build
7e8659d
@jclem
Call resolve, then require
7e12bd7
@jclem
Add Webpack comment vis-a-vis eval
b0e12e7
@jclem jclem changed the title Wrap require to support relative requires Support installed npm modules and relative require Apr 21, 2021
@jclem
Remove caveat about node_modules
01f87b6
@joshmgross joshmgross mentioned this pull request Apr 21, 2021
Make require relative to GitHub workspace #116
Closed
@jclem
Add new integration tests
3ede589
joshmgross
joshmgross approved these changes Apr 21, 2021
View reviewed changes
Comment thread src/wrap-require.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.
jclem and others added 6 commits April 21, 2021 17:39
@jclem
Add integration test running
19e7914
@jclem
Add actions/checkout
4a93ad9
@jclem
Add npm ci to integration test
fb7e8f7
@jclem
Workflow syntax error
5ee517d
@jclem @joshmgross
Update src/wrap-require.ts
256da4e 
Co-authored-by: Josh Gross <[email protected]>
@jclem
v4.0.0
c758586
jclem added 2 commits April 21, 2021 17:48
@jclem
Merge branch 'wrap-require' of https://github.com/actions/github-script…
b616178 
… into wrap-require
@jclem
Run build script
2923e50
@jclem
jclem merged commit 95fb649 into main Apr 21, 2021
@jclem
jclem deleted the wrap-require branch April 21, 2021 21:50
JamesMGreene
JamesMGreene reviewed Apr 21, 2021
View reviewed changes
Comment thread src/wrap-require.ts
{
// Webpack does not have an escape hatch for getting the actual
// module, other than `eval`.
paths: eval('module').paths.concat(process.cwd())

@JamesMGreene JamesMGreene Apr 21, 2021

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jclem jclem Apr 21, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
JamesMGreene reacted with heart emoji
All reactions
  • ❤️ 1 reaction
Comment thread src/wrap-require.ts
}

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

@JamesMGreene JamesMGreene Apr 21, 2021

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jclem jclem Apr 21, 2021

Copy link
Copy Markdown
Contributor Author

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:

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
JamesMGreene reacted with heart emoji
All reactions
  • ❤️ 1 reaction

@jclem jclem Apr 21, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@JamesMGreene JamesMGreene Apr 21, 2021

Copy link
Copy Markdown

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jclem jclem mentioned this pull request Apr 21, 2021
Only search cwd on user-script require calls #136
Merged
@JamesMGreene JamesMGreene mentioned this pull request Jul 15, 2021
Support importing ESM files #168
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@JamesMGreene JamesMGreene JamesMGreene left review comments
@joshmgross joshmgross joshmgross approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@jclem @JamesMGreene @joshmgross
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.