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.

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

Module error when using Javascript #9

Open
FullStackIndie opened this issue Oct 5, 2024 · 4 comments
Open

Module error when using Javascript #9

FullStackIndie opened this issue Oct 5, 2024 · 4 comments
Assignees
Labels
area/backend Needs backend code changes bug Something isn't working

Comments

@FullStackIndie
Copy link

FullStackIndie commented Oct 5, 2024

I keep getting error when using the Javascript module and using

  • id: run_nodejs_script
    type: io.kestra.plugin.scripts.node.Script
    beforeCommands:
    • npm i @kestra-io/libs
      taskRunner:
      type: io.kestra.plugin.scripts.runner.docker.Docker
      containerImage: node:slim
      warningOnStdErr: false
      logLevel: DEBUG
      script: |
      const Kestra = require ('@kestra-io/libs');

here is the error
2024-10-05 15:27:15.985Using task runner 'io.kestra.plugin.scripts.runner.docker.Docker' 2024-10-05 15:27:19.214Starting command with container id 3dbbecd088494911a2f47b8093bf5a3c45b8f0afc0e973790788d2e9d9c447f0 [/bin/sh -c set -e npm i @kestra-io/libs node /tmp/kestra-wd/tmp/1ZqEAZLo7y9Y3IJlth2u32/4712153262335601460.js] 2024-10-05 15:27:20.722 2024-10-05 15:27:20.722added 1 package in 1s 2024-10-05 15:27:20.724npm notice New minor version of npm available! 10.8.3 -> 10.9.0 2024-10-05 15:27:20.724npm notice 2024-10-05 15:27:20.724npm notice To update run: npm install -g [email protected] 2024-10-05 15:27:20.724npm notice Changelog: https://github.com/npm/cli/releases/tag/v10.9.0 2024-10-05 15:27:20.725npm notice 2024-10-05 15:27:20.762/tmp/kestra-wd/tmp/1ZqEAZLo7y9Y3IJlth2u32/4712153262335601460.js:315 2024-10-05 15:27:20.763at TracingChannel.traceSync (node:diagnostics_channel:315:14) 2024-10-05 15:27:20.763code: 'ERR_REQUIRE_ESM' 2024-10-05 15:27:20.763Instead either rename index.js to end in .cjs, change the requiring code to use dynamic import() which is available in all CommonJS modules, or change "type": "module" to "type": "commonjs" in /tmp/kestra-wd/tmp/1ZqEAZLo7y9Y3IJlth2u32/node_modules/@kestra-io/libs/package.json to treat all .js files as CommonJS (using .mjs for all ES modules instead). 2024-10-05 15:27:20.763index.js is treated as an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which declares all .js files in that package scope as ES modules. 2024-10-05 15:27:20.763at Object.<anonymous> (/tmp/kestra-wd/tmp/1ZqEAZLo7y9Y3IJlth2u32/4712153262335601460.js:1:16) { 2024-10-05 15:27:20.763} 2024-10-05 15:27:20.763undefined 2024-10-05 15:27:20.763

I tried using the require and import ES6 syntax and both did not work.

here is the error when using import

2024-10-05 15:29:19.621added 1 package in 846ms 2024-10-05 15:29:19.623npm notice 2024-10-05 15:29:19.623npm notice New minor version of npm available! 10.8.3 -> 10.9.0 2024-10-05 15:29:19.623npm notice Changelog: https://github.com/npm/cli/releases/tag/v10.9.0 2024-10-05 15:29:19.623npm notice To update run: npm install -g [email protected] 2024-10-05 15:29:19.623npm notice 2024-10-05 15:29:19.674Reparsing as ES module because module syntax was detected. This incurs a performance overhead. 2024-10-05 15:29:19.674To eliminate this warning, add "type": "module" to /tmp/kestra-wd/tmp/1HNjMnlv4YbIleA7jXXEfV/package.json. 2024-10-05 15:29:19.674(Usenode --trace-warnings ...to show where the warning was created) 2024-10-05 15:29:19.674(node:18) [MODULE_TYPELESS_PACKAGE_JSON] Warning: Module type of file:///tmp/kestra-wd/tmp/1HNjMnlv4YbIleA7jXXEfV/2272223555352646281.js is not specified and it doesn't parse as CommonJS. 2024-10-05 15:29:19.679^^^^^^ 2024-10-05 15:29:19.679at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:483:26) 2024-10-05 15:29:19.679SyntaxError: The requested module '@kestra-io/libs' does not provide an export named 'Kestra' 2024-10-05 15:29:19.679at async ModuleJob.run (node:internal/modules/esm/module_job:254:5) 2024-10-05 15:29:19.679file:///tmp/kestra-wd/tmp/1HNjMnlv4YbIleA7jXXEfV/2272223555352646281.js:1 2024-10-05 15:29:19.679import {Kestra} from '@kestra-io/libs'; 2024-10-05 15:29:19.679at ModuleJob._instantiate (node:internal/modules/esm/module_job:171:21) 2024-10-05 15:29:19.683Node.js v22.9.0

Since I was following Kestra documentation for JavaScript plugin npm will default to the latest package which is currently 0.20.0-SNAPSHOT

The temporary fix is to use the previous version in npm explicitly - "npm i @kestra-io/[email protected]"
To fix it, it looks like the code should be updated to ES6 syntax, remove type:"module" from the package.json, or rename index.js to index.cjs in order to continue using CommonJS with "type":"module" in package.json

@kestrabot kestrabot bot added this to Issues Oct 5, 2024
@github-project-automation github-project-automation bot moved this to Backlog in Issues Oct 5, 2024
@anna-geller anna-geller added bug Something isn't working area/backend Needs backend code changes labels Oct 10, 2024
@smunteankestra
Copy link

I am encountering an error when attempting to run the following Kestra flow, which is designed to execute a Node.js script using the @kestra-io/libs library within a Docker container.

id: myflow
namespace: company.team
tasks:
  - id: run_nodejs_script
    type: io.kestra.plugin.scripts.node.Script
    beforeCommands:
      - npm i @kestra-io/libs
    taskRunner:
      type: io.kestra.plugin.scripts.runner.docker.Docker
      image: node:slim
    warningOnStdErr: false
    logLevel: DEBUG
    script: |
      const Kestra = require('@kestra-io/libs');

error:

2024-10-14 10:00:57.059 npm notice New minor version of npm available! 10.8.3 -> 10.9.0
2024-10-14 10:00:57.094 /tmp/kestra-wd/tmp/2b3i1WNwnR38aPXbsHy4iy/12344889282504685718.js:315
2024-10-14 10:00:57.095 Error [ERR_REQUIRE_ESM]: require() of ES Module /tmp/kestra-wd/tmp/2b3i1WNwnR38aPXbsHy4iy/node_modules/@kestra-io/libs/src/index.js from /tmp/kestra-wd/tmp/2b3i1WNwnR38aPXbsHy4iy/12344889282504685718.js not supported.
2024-10-14 10:00:57.096 index.js is treated as an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which declares all .js files in that package scope as ES modules.
2024-10-14 10:00:57.097 Instead, either rename index.js to end in .cjs, change the requiring code to use dynamic import(), or change "type": "module" to "type": "commonjs" in /tmp/kestra-wd/tmp/2b3i1WNwnR38aPXbsHy4iy/node_modules/@kestra-io/libs/package.json to treat all .js files as CommonJS.
2024-10-14 10:00:57.099 Node.js v22.9.0
2024-10-14 10:00:57.284 Command failed with exit code 1

@wrussell1999
Copy link
Member

Still seems like an issue in 0.20: kestra-io/plugin-scripts#192

@m-t-a97
Copy link
Contributor

m-t-a97 commented Jan 15, 2025

@wrussell1999 @anna-geller, let's continue the conversation here from kestra-io/plugin-scripts#192.

@m-t-a97
Copy link
Contributor

m-t-a97 commented Jan 15, 2025

First of all, the approach in the the current PR to resolve the issue has some potential problems and doesn't fully address the issue here or provide a future-proof solution.

Mixing Module Formats

The solution proposed attempts to provide support for both CommonJS and ESM modules. Changing file extensions to .cjs locks the library into being used primarily as a CommonJS module. This might break compatibility for users expecting .js or ESM-friendly files. Also this can lead to maintenance challenges:

  • Maintaining both formats increases the complexity of the codebase, especially for developers unfamiliar with the setup.
  • If updates are made in one format but not the other, it could lead to inconsistencies and bugs.
  • Assumption has been made that setting "type": "module" in package.json causes the error, and this change was made for Jest testing. This is incorrect as Jest wasn't properly configured to begin with e.g. with babel-jest etc. Jest can run in both ESM and CJS mode.

Package.json Configuration

The issue stems from setting "type": "module" in package.json, which forces all .js files to be interpreted as ES modules. This breaks compatibility with CommonJS. So changing file extensions to .cjs to retain CommonJS functionality is a workaround, not a clean solution. A better solution would involve configuring build tooling (e.g., Esbuild, Rollup) to transpile and bundle code for both module systems to distribute separate bundles for CJS and ESM so that users who want to use either formats are free to do so. This is how other popular npm packages are doing this.

Compatibility with Previous Versions

By requiring developers to specify the @kestra-io/[email protected] version explicitly, you're tying compatibility to a specific library version. This approach has the following issues:

  • Limits Flexibility: Users are locked to a specific version unless they refactor their code for compatibility with newer versions.
  • Indicates Lack of Backward Compatibility: It suggests the library may not follow semantic versioning or maintain backward compatibility. The PR for example converts the Kestra function into a class which means the user will have to instantiate an object of the class in order to use it. Whereas the current approach is a default exported function that can be used without an object reference etc. So the suggested changes breaks compatibility with any existing code users have.

Testing and QA Process

The testing process for this is lacking as it doesn't highlight the changes made working with an external project:

  • Local Testing Only: running jest tests in the lib itself is a good first step to ensure the code is working as expected but this is a library project being consumed by other projects who may be using either ESM or CJS so a more robust testing procedure must be in place.
  • No Integration Tests: there's no mention of testing the library in real-world scenarios e.g. testing this library against an external JS/TS project importing this code etc. For example there is no evidence as to whether these changes made actually work. How do we know that this library is now working with another project that is using CJS/ESM?

m-t-a97 added a commit to m-t-a97/libs that referenced this issue Jan 15, 2025
m-t-a97 added a commit to m-t-a97/libs that referenced this issue Jan 16, 2025
fix: Removed my username from author
elevatebart added a commit that referenced this issue Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/backend Needs backend code changes bug Something isn't working
Projects
Status: Backlog
Development

No branches or pull requests

5 participants