Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Support importing ESM files #168

Copy link
Copy link
Closed
@JamesMGreene

Description

@JamesMGreene
Issue body actions

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:

    Workflow job statuses

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 🤷🏻‍♂️

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or requestNew feature or request

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

      Morty Proxy This is a proxified and sanitized view of the page, visit original site.