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

Use npm in favor of yarn #63

Closed
wants to merge 8 commits into from
Closed

Use npm in favor of yarn #63

wants to merge 8 commits into from

Conversation

vintidui
Copy link

@vintidui vintidui commented Apr 15, 2024

Issue

#64

Description

This is my first contribution in an effort to improve the universality (use across different operating systems) and ease of use for the Morepheus client. People should be able to check out the repo and get it running with a command or two and no modifications to their local machine.

yarn has given me headaches at a few companies and npm is tried and true. npm lags behind with nifty new features but is bundled with node.js and usually catches up for the important stuff. A nice benefit is we won't need devs to install a separate global dependency to run the project.

Proof of Contribution

Requested Weights: 8000
MorpheusAIs/Docs#202

@@ -5,14 +5,13 @@
"description": "Morpheus is private, sovereign, AI",
"main": ".webpack/main",
"scripts": {
"preinstall": "node .setup/yarn-preinstall-system-validation.mjs",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This solution was added in the following PR: #59
It addressed the following issue: #58
I tested these changes with python 3.12.2 and was able to start, package, and make.


Windows OS Systems
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing README only mentions Windows when the instructions are similar for all operating systems. These changes generalize the README so it's apparent that Morpheus works for more than only Windows.

@vintidui vintidui marked this pull request as ready for review April 19, 2024 03:54
@@ -62,6 +61,7 @@
"@typescript-eslint/eslint-plugin": "^5.0.0",
"@typescript-eslint/parser": "^5.0.0",
"@vercel/webpack-asset-relocator-loader": "1.7.3",
"appdmg": "^0.6.6",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional dependency of:

└─┬ @electron-forge/[email protected]
       └─┬ [email protected]
              └── [email protected] deduped

Since it's optional, it's not guaranteed to be included, but needed for npm run make. I was getting the following error without it:

An unhandled rejection has occurred inside Forge:
Error: Cannot find module 'appdmg'

Adding explicit dependency for convenience.

https://github.com/electron-userland/electron-installer-dmg/blob/ddc6db7a9a90c1cf19db1454bd41ae8a6d1fe106/package.json#L35

@betterbrand
Copy link
Contributor

Hi @vintidui! There's a lot going on here. First, this pr is addressing multiple factors, some of which weren't asked for, and some of which aren't passing build tests. Second, the request for 8000 weights seems to be based on a weight value other than what the current value is noted in the latest weights guide. 8000 weights is $3,256.

Let's break this PR into different issues and discuss them before work is done.

  1. Yarn. There are reasons not to rip it out suddenly, and affects the verbiage removed from the readme.
  2. Appdmg. It's causing other OS builds to fail. Makes sense, but causing problems.
  3. The builds are failing.

I'm closing this PR, but not the effort. Please reach out on Discord to discuss these changes and align on pricing.

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

Successfully merging this pull request may close these issues.

2 participants