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

fix(cli): move @faker-js/faker to dependencies #1801

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

pineapplehunter
Copy link
Contributor

@faker-js/faker is needed to run the CLI

PR Checklist

If you have any questions, you can refer to the Contributing Guide

What is the current behavior?

Expected: The app should run with deps installed by yarn install --prod.
Current: Crashes

Log file

--ignore-engines is removed from the log because it should not matter.

$ yarn install && yarn build
$ node bin/index.js 
Usage: mqttx [options] [command]
...
  ls [options]        List information based on the provided options.
  help [command]      display help for command
$ yarn install --prod
$ node bin/index.js
node:internal/modules/cjs/loader:1148
  throw err;
  ^

Error: Cannot find module '@faker-js/faker'
Require stack:
- /home/shogo/tmp/MQTTX/cli/dist/src/utils/simulate.js
- /home/shogo/tmp/MQTTX/cli/dist/src/utils/parse.js
- /home/shogo/tmp/MQTTX/cli/dist/src/index.js
- /home/shogo/tmp/MQTTX/cli/bin/index.js
    at Module._resolveFilename (node:internal/modules/cjs/loader:1145:15)
    at Module._load (node:internal/modules/cjs/loader:986:27)
    at Module.require (node:internal/modules/cjs/loader:1233:19)
    at require (node:internal/modules/helpers:179:18)
    at Object.<anonymous> (/home/shogo/tmp/MQTTX/cli/dist/src/utils/simulate.js:38:15)
    at Module._compile (node:internal/modules/cjs/loader:1358:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1416:10)
    at Module.load (node:internal/modules/cjs/loader:1208:32)
    at Module._load (node:internal/modules/cjs/loader:1024:12)
    at Module.require (node:internal/modules/cjs/loader:1233:19) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/home/shogo/tmp/MQTTX/cli/dist/src/utils/simulate.js',
    '/home/shogo/tmp/MQTTX/cli/dist/src/utils/parse.js',
    '/home/shogo/tmp/MQTTX/cli/dist/src/index.js',
    '/home/shogo/tmp/MQTTX/cli/bin/index.js'
  ]
}

Node.js v20.16.0

Issue Number

None

What is the new behavior?

CLI works wihtout devDependencies

Does this PR introduce a breaking change?

  • Yes
  • No

Specific Instructions

Are there any specific instructions or things that should be known prior to review?

Other information

faker is needed to run the CLI
@ysfscream ysfscream self-requested a review November 12, 2024 14:44
@ysfscream
Copy link
Member

This pull request includes a change to the cli/package.json file, which involves moving the @faker-js/faker package from devDependencies to dependencies.

Dependency management:

  • Moved @faker-js/faker from devDependencies to dependencies in cli/package.json. [1] [2]

@ysfscream ysfscream requested a review from Red-Asuka November 12, 2024 14:44
@ysfscream ysfscream added dependencies Pull requests that update a dependency file chore Changes in build tools or dependent packages CLI MQTTX CLI labels Nov 12, 2024
@ysfscream ysfscream added this to the v1.11.1 milestone Nov 12, 2024
@ysfscream
Copy link
Member

image

Hi, @pineapplehunter. Thanks for your PR! However, the official website of faker.js recommends installing it as a development dependency. Why was it moved to dependencies? Is there any issue or reason for this change?

@ysfscream ysfscream added the discussion This issues and pr are worth discussing label Nov 12, 2024
@pineapplehunter
Copy link
Contributor Author

I believe it is recommended on the docs to install it as a devDependency because it is often used in tests and not used in the final package.
In this case, mqttx cli is using faker.js in the final package (for mqttx simulate?), so it seems better to include it in dependencies.

I am packaging mqttx cli in nixpkgs (NixOS/nixpkgs#337730), and it is running yarn install --prod before packaging to reduce the size of the final output.
I made this change because if faker.js is in devDependencies it would get deleted in the process.

@ysfscream ysfscream merged commit 3d89c3a into emqx:main Nov 13, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Changes in build tools or dependent packages CLI MQTTX CLI dependencies Pull requests that update a dependency file discussion This issues and pr are worth discussing
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants