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

Cannot use JS bundler because of shelljs dependency #942

Closed
tjosepo opened this issue May 24, 2023 · 14 comments
Closed

Cannot use JS bundler because of shelljs dependency #942

tjosepo opened this issue May 24, 2023 · 14 comments

Comments

@tjosepo
Copy link

tjosepo commented May 24, 2023

Please check our current Issues to see if someone already reported this https://github.com/Microsoft/azure-pipelines-task-lib/issues

Environment

azure-pipelines-task-lib version: 4.3.1

Issue Description

JavaScript tasks cannot be bundled using a library like rollup or esbuild because shelljs does not explicitely declare its dependencies: shelljs/shelljs#962

There is currently a pull-request open on shelljs to address the issue, but it hasn't moved since March shelljs/shelljs#1119

Currently, the best way I found to circumvent this is to create a bundler plugin to replace the /node_modules/shelljs/shell.js file with the fixed file at compile-time.

Expected behaviour

I can bundle a task using a popular bundler like rollup or esbuild with the target platform set to node.

Actual behaviour

The task will build but most shelljs files will be missing from the bundle.

There will be a runtime exception when trying to run the task due to missing dependencies that only resolve at runtime.

Steps to reproduce

Using esbuild:

  1. Add esbuild as a devDependency:
npm i --D esbuild
  1. Create a build.mjs file with this content:
import * as esbuild from 'esbuild'

await esbuild.build({
  entryPoints: ['./src/index.js'], // or whatever your entryfile is...
  outfile: './dist/index.js',
  bundle: true,
  platform: "node",
  target: "node16",
  allowOverwrite: true,
});
  1. Run the build:
node build.mjs
  1. Run the task:
node dist/index.js
  1. An error message should appear.

Logs

$ node dist/index.js
node:internal/modules/cjs/loader:1029
  throw err;
  ^

Error: Cannot find module './src/cat'
Require stack:
- /dist/index.js
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:1026:15)
    at Function.Module._load (node:internal/modules/cjs/loader:871:27)
    at Module.require (node:internal/modules/cjs/loader:1098:19)
    at require (node:internal/modules/cjs/helpers:108:18)
    at /dist/index.js:2807:7
    at Array.forEach (<anonymous>)
    at node_modules/shelljs/shell.js (/dist/index.js:2806:24)
    at __require (/dist/index.js:12:52)
    at node_modules/azure-pipelines-task-lib/task.js (/dist/index.js:7287:17)
    at __require (/dist/index.js:12:52) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/dist/index.js'
  ]
}
@ivanduplenskikh
Copy link
Contributor

Hi @tjosepo thanks for reporting! We are working on more prioritized issues at the moment, but will get back to this one soon.

@github-actions
Copy link

This issue has had no activity in 90 days. Please comment if it is not actually stale

@github-actions github-actions bot added the stale label Aug 23, 2023
@dnitsch
Copy link

dnitsch commented Aug 29, 2023

@tjosepo could you share an example of your workaround please

@github-actions github-actions bot removed the stale label Aug 29, 2023
@Everspace
Copy link

Everspace commented Sep 18, 2023

@tjosepo I have specified a resolutions in my package.json since I use yarn.

  "resolutions": {
    "shelljs": "Everspace/shelljs#b364c45"
  },

But the more correct/npm way is

  "dependencies": {
    "shelljs": "Everspace/shelljs#b364c45"
  },
  "overrides": {
    "shelljs": "$shelljs"
  }

Copy link

This issue has had no activity in 90 days. Please comment if it is not actually stale

@github-actions github-actions bot added the stale label Dec 17, 2023
@Everspace
Copy link

not stale.

@github-actions github-actions bot removed the stale label Dec 17, 2023
@pippolino
Copy link

UP

rbbarad added a commit to aws/aws-toolkit-azure-devops that referenced this issue Jan 22, 2024
…g issue (#539)

### Background

This is a follow-up pr to fix the issue in #522. 

[Mark shelljs as external for esbuild bundling](#537)  attempted to address this issue, but was not a successful solution for all of our tasks, as some of our tasks need the commands present in ShellJS.

### Problem

This issue emerged with the bump of `azure-pipelines-task-lib` dependency in: #473.

Problem detailed in the open issue on `azure-pipelines-task-lib` repo: [Cannot use JS bundler because of shelljs dependency](microsoft/azure-pipelines-task-lib#942).

Essentially, the toolkit takes a dependency on `azure-pipelines-task-lib` which takes an additional dependency on ShellJS. Our toolkit uses the esbuild JS Bundler to build the extension. This fails to bundle ShellJS because it uses dynamic require statements ([problematic ShellJS code](https://github.com/shelljs/shelljs/blob/a6d1e49f180a495d83bcf67bd85782c626aae029/shell.js#L23-L26)) instead of explicit require statements, which prevents bundlers from correctly analyzing the dependencies.
-  In short, require('./a') works but require('./' + 'a') doesn't.

[ShellJS has no plans to accommodate these explicit requires statements](shelljs/shelljs#962 (comment))

### Solution

Before making any larger scale changes, let's get `azure-pipelines-task-lib` back to its last known healthy state (working version we used for last release). This PR addresses that.

### Future `azure-pipelines-task-lib` updates
Whenever we next bump the `azure-pipelines-task-lib` major version, we'll need to keep an eye on updates to microsoft/azure-pipelines-task-lib#942 for any new fixes or workarounds. Current known workarounds involve forking the ShellJS repository, making the necessary changes to ShellJS, and overriding the ShellJS dependency in `package.json` with the forked version.
- [Example of forked ShellJS with hardcoded explicit `requires` statements](Everspace/shelljs@4e2ea23)
- [Example of creating a bundler plugin with a fixed ShellJS file at compile time](https://github.com/tjosepo/azure-pipelines/blob/main/packages/esbuild-plugin-azure-pipelines-task-lib-fix/src/index.js)
@rbbarad
Copy link

rbbarad commented Jan 22, 2024

Please prioritize this issue if you can. This issue is preventing us from updating to recent versions of azure-pipelines-task-lib. As mentioned in the issue description, ShellJS does not plan on fixing this issue. Therefore a workaround will be needed to continue using a bundler with azure-pipelines-task-lib

@ivanduplenskikh
Copy link
Contributor

@tjosepo
I've reproduced a problem with your example.

As a temporary solution you can use esbuild API external option:

import * as esbuild from 'esbuild'

await esbuild.build({
  entryPoints: ['./src/index.js'], // or whatever your entryfile is...
  outfile: './dist/index.js',
  bundle: true,
  platform: "node",
  target: "node16",
  allowOverwrite: true,
  external: ["shelljs"] // After adding this option the error no longer occurs
});

@ivanduplenskikh
Copy link
Contributor

Also, I found closed issue in esbuild repository with a solution like as a comment above

@ivanduplenskikh
Copy link
Contributor

I'm closing this issue.
Feel free to reopen it if you have questions about it.

@7PH
Copy link

7PH commented May 24, 2024

Hey @ivanduplenskikh , putting shelljs in external is not a solution that will work all the time, and when it does not, we have to provide shelljs in node_modules which mitigates the benefit of having a tree-shaked bundle. Since we have a size limit for VSIX files, I would argue that this is a workaround and not a solution.

Is there any plan to address this? I see there were attempts at dropping shelljs in the past, for instance.

@pippolino
Copy link

pippolino commented May 25, 2024

I tried to force the Shelljs modules to load. The problem is not occurring and for the moment it works.

It was used in the dependency-check for Azure DevOps library, see commit

@7PH
Copy link

7PH commented May 27, 2024

See POC repository for a workaround which:

  • Puts shelljs in externals for esbuild
  • Puts an empty node_modules/shelljs/index.js file so the require(..) doesn't fail

It will work if you don't directly use methods from azure-pipelines-task-lib which actually use shelljs functions.

Feel free to comment there if you find a better solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants