Support importing ESM files

[JamesMGreene]

Is your feature request related to a problem? Please describe.

This Action recently added support for importing CommonJS-based npm packages and local modules within the script body using an monkey-patched require(...) method. That is awesome! 🎉

Alas, the Node.js community has been moving away from CommonJS more and more now that the ECMAScript Modules (ESM) standard is out of its experimental status.

The good news? If your module/application is written with ESM, it can import both other ESM files and CommonJS modules using the new import keyword, or a dynamic import(...) method call. 💚

// Syntax-sugared import statements
import { has } from 'lodash-es'  // An ESM package
import { getOctokit } from '@actions/github'  // A CommonJS package

// Dynamic import method call
const { setOutput } = await import('@actions/core')  // A CommonJS package

The bad news? If your module/application is written with CommonJS, the require(...) method can only import other CommonJS modules. It is not possible to directly import ESM files because they are not guaranteed to be synchronous, which is a requirement of the require(...) approach. The import keyword is not available within CommonJS files but it is possible to use the dynamic import(...) approach instead. However, the import method is not currently accessible within github-script blocks — I tried that and it failed! ❌

Unfortunately, this means that I can no longer utilize this Action's handy require(...) behavior on my own application's local modules as we recently converted to ESM (after becoming blocked by a dependency that converted to ESM 😬), e.g. github/docs@.github/workflows/staging-deploy-pr.yml#L52-L61 😭

Describe the solution you'd like

In an ideal world, I think we probably should have only added support for the ESM approach to this module such that we could've supported both. Alas, timing and knowledge such as they were, we opted for the standard CommonJS require(...) approach.

Unfortunately, this would prevent us from adding full support for ESM's import keyword now, or at least not without at least bumping this Action's major version number to indicate the breaking change.

As such, I think a more reasonable compromise would be to just add support for the dynamic import(...) method calls within the current wrapping function.

Describe alternatives you've considered

I did some fairly extensive testing of a handful of approaches and potential workarounds to this situation:

   

You can see the implementation details in this workflow file.

The failed job above ☝🏻 that I would like to see passing with the proposed solution is the "Try ESM dynamic imports" (esm-dynamic-imports) job.

Alternative (1):

I was only able to get 1 of the 6 attempts working with the current state of the github-script Action, which involves using the esm npm package and some questionable additional workarounds that may or may not work when applied to more advanced ESM dependency chains. 🤷🏻‍♂️

Alternative (2):

Not use actions/github-script and just run a roughly equivalent Node.js script that uses @actions/core, etc. 😢

Additional context

N/A 🤷🏻‍♂️

[github-actions]

This issue is stale because it has been open for 60 days with no activity. Remove the "Stale" label or comment on the issue, or it will be closed in 7 days.

[LongLiveCHIEF]

commenting to prevent auto-removal.

Will this be resolved when node 16 goes LTS? (not sure what github actions will be doing with node on Ubunut runners)

[JamesMGreene]

@LongLiveCHIEF That's a good point, having Node.js 16.x in place on the runners would probably be the first step necessary. 🤔

After that, there might still be more work (if possible? 😬) to reconfigure the dynamic import method to utilize the appropriate relative pathing as we did with require in PR #116.

[ethomson]

It's not an issue of the runner image, but the runner itself. The runner bundles its own node because you may have a random version by default (because you ran actions/setup-node) or may not have node at all (in the case of self-hosted, or when running in a container). Currently that's node12.

We'll add node16 as an option 🔜 at which point this becomes more feasible. I think that if we update actions/github-script in a new major version to use the node16 runner that this may "just work"?

[github-actions]

This issue is stale because it has been open for 60 days with no activity. Remove the "Stale" label or comment on the issue, or it will be closed in 7 days.

[JamesMGreene]

@ethomson

I think that if we update actions/github-script in a new major version to use the node16 runner that this may "just work"?

This would be awesome but it may be wishful thinking. 😅 When this action was updated to support require for local/installed modules, it needed some tricky patching to utilize the correct paths. 🤔

[JamesMGreene]

JavaScript Actions can now run using Node.js 16 by changing the action.yml file's runs.using configuration from using: node12 to using: node16: https://docs.github.com/en/actions/creating-actions/metadata-syntax-for-github-actions#example-using-nodejs-v16

I imagine this means we can now investigate if switching to using: node16 makes ESM imports possible -- at least via the dynamic import(...) function. 🙃

[joshmgross]

With #235 and actions/github-script using Node 16, it looks like this should Just Work™️

https://github.com/joshmgross/actions-testing/runs/5130290014?check_suite_focus=true

Workflow:

      - uses: thboop/github-script@d2ed94b14f4e194b2afdd7fe2af49e0d93c42d92
        with:
          script: |
            try {
              console.log('Before dynamic import')
              const { default: printStuff } = await import('${{ github.workspace }}/src/print-stuff.js')
              console.log('After dynamic import')
              await printStuff()
              console.log('At the end')
            } catch (error) {
              console.error('Threw an error!')
              throw error
            }

package.json

{
   "name": "",
  "description": "",
  "type": "module",
  "private": true,
  "version": "0.1.0",
  "dependencies": {},
  "devDependencies": {}
}

src/print-stuff.js

export default function printStuff() { console.log('stuff') }

Since we're not wrapping import like we are with require (#135), I'm not sure how it would work with installed NPM modules and we need to specify the full absolute path using the workspace variable await import('${{ github.workspace }}/src/print-stuff.js')

Thanks @JamesMGreene for all the help