Skip to content

Use "node --run" instead of "npm run" #3764

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

Merged
merged 3 commits into from
May 6, 2025

Conversation

KristjanESPERANTO
Copy link
Collaborator

This has the advantage that the package manager is no longer involved after the installation process.

However, previous start commands such as npm run start continue to work. So we don't even have to adapt the documentation.

… package manager is no longer involved after the installation process.
@sdetweil
Copy link
Collaborator

what advantage is that for the user?

they just want the thing to run

@khassel
Copy link
Collaborator

khassel commented Apr 13, 2025

I see it as cleanup and restriction to the real use cases:

  • npm only for install/update dependencies
  • node for running scripts

There is no advantage or disadvantage for the user.

@sdetweil
Copy link
Collaborator

sdetweil commented Apr 13, 2025

so why add more confusion over what to do when?

i will advise no one to use this. and advise against

@KristjanESPERANTO
Copy link
Collaborator Author

I see the arguments against it and it's okay if we decide against it for now.

But for the user I see also these advantages:

  1. it is more intuitive for users. Why should I use a package manager to start a script?

  2. it reduces complexity. Which is helpful for understanding and also for troubleshooting, as the package manager no longer plays a role after installation. The user may therefore be able to solve their problem themselves more quickly or receive help more quickly.

@sdetweil
Copy link
Collaborator

sdetweil commented Apr 13, 2025

the users dont know what a package manager is,
we have successfully graduated to they dont care about the technicalities of the base. witness the success of the docker deployment(almost, due to update issues for modules). they just want it to run!

they want to create an info panel. 90% of that content isn't ours at all.

lets focus on making it run with as few crazy options as possible. my objective is to make manual install,
upgrade and configuration including modules, go away

@KristjanESPERANTO
Copy link
Collaborator Author

KristjanESPERANTO commented Apr 14, 2025

the users

I think we have a very different picture of who “the users” are and what they want.

the users dont know what a package manager is,

It doesn't take magic to learn it.

they just want it to run!

But simple consumers are not our main target group. As I interpret our manifesto, we want to inspire people to learn, try things out, break stuff, create things and get involved 💥

If we confuse users when they come across both options, so much the better, then they will question it and learn. It's not as if the change would be a breaking change.

And if a user just wants to be a simple consumer, he can use a third party installation script and a module manager such as mmpm or even buy a system on Amazon.

In any case, a simple user won't care whether we use “npm run” or "node --run". An "old" user will use the old command, a "new" user the new one.

my objective is to make manual install, upgrade and configuration including modules, go away

The question is whether we need to do this in the core? The discussion about a graphical backend in the core (#3749) did not go in that direction, for example.


I think it would be good if we looked at the change mainly from a technical point of view. And as I understand it, there are fewer arguments against it than for it. But it is like a polish, the car will run without it and if there are others who think we shouldn't do it, I can live without this "polish" 🙂

@sdetweil
Copy link
Collaborator

sdetweil commented Apr 14, 2025

i see the manifesto. i see the user community
there is still a ton to learn to make an info panel solution.

these are not simple consumers. just not wanting to become developers

npm has been 0.0001% of the support issues.

as for my goal, i didn't say HOW i would accomplish that. i haven't asked for any function from the base

@KristjanESPERANTO KristjanESPERANTO requested a review from rejas May 5, 2025 15:30
@rejas
Copy link
Collaborator

rejas commented May 6, 2025

I like this polish (although I can also understand the "dont change if it aint broke" mentality) and it seems to be the way forward.
Still would like to get either @khassel or @sdetweil approval before merging it.

@sdetweil
Copy link
Collaborator

sdetweil commented May 6, 2025

i disagree with this pr. we have all kinds of other problems to solve without this unnecessary 'polish'

@bugsounet
Copy link
Contributor

For once, i agree with @sdetweil

@KristjanESPERANTO
Copy link
Collaborator Author

we have all kinds of other problems ...

There are always more important things, but that has nothing to do with this PR. I haven't heard any concrete technical arguments against it. node --run is a new feature that node offers us, why shouldn't we use it?

without this unnecessary 'polish'

If I want to use MM independently from npm, it is not unnecessary.

For once, i agree with @sdetweil

Why?

@khassel
Copy link
Collaborator

khassel commented May 6, 2025

As I already stated above:

There is no advantage or disadvantage for the user.

I think we should use node wherever possible. For me, it's cleaner not to use a package manager to launch scripts.

@rejas
Copy link
Collaborator

rejas commented May 6, 2025

Ok, so I will merge it. Sams arguments are noted, but I agree with the other two more. lets hope node doesnt prove us wrong.

@rejas rejas merged commit b9d63d7 into MagicMirrorOrg:develop May 6, 2025
9 checks passed
@KristjanESPERANTO
Copy link
Collaborator Author

Thanks guys! And if it turns out to be a wrong/bad decision, I will be happy to help reverse it. Nothing is set in stone.

@KristjanESPERANTO KristjanESPERANTO deleted the node-run branch May 6, 2025 21:01
rejas pushed a commit that referenced this pull request May 7, 2025
This will fix #3772 caused by #3764.

Since I work with `start:wayland:dev` instead of `start:dev`, I didn't
notice this, sorry.
@khassel khassel mentioned this pull request Jun 30, 2025
khassel added a commit that referenced this pull request Jun 30, 2025
## [2.32.0] - 2025-07-01

Thanks to: @bughaver, @bugsounet, @khassel, @KristjanESPERANTO,
@plebcity, @rejas, @sdetweil.

> ⚠️ This release needs nodejs version `v22.14.0 or higher`

### Added

- [config] Allow to change module order for final renderer (or
dynamically with CSS): Feature `order` in config (#3762)
- [clock] Added option 'disableNextEvent' to hide next sun event (#3769)
- [clock] Implement short syntax for clock week (#3775)

### Changed

- [refactor] Simplify module loading process (#3766)
- Use `node --run` instead of `npm run` (#3764) and adapt `start:dev`
script (#3773)
- [workflow] Run linter and spellcheck with LTS node version (#3767)
- [workflow] Split "Run test" step into two steps for more clarity
(#3767)
- [linter] Review linter setup (#3783)
  - Fix command to lint markdown in `CONTRIBUTING.md`
  - Re-activate JSDoc linting and fix linting issues
  - Refactor ESLint config to use `defineConfig` and `globalIgnores`
  - Replace `eslint-plugin-import` with `eslint-plugin-import-x`
- Switch Stylelint config to flat format and simplify Stylelint scripts
- [workflow] Replace Node.js version v23 with v24 (#3770)
- [refactor] Replace deprecated constants `fs.F_OK` and `fs.R_OK`
(#3789)
- [refactor] Replace `ansis` with built-in function `util.styleText`
(#3793)
- [core] Integrate stuff from `vendor` and `fonts` folders into main
`package.json`, simplifies install and maintaining dependencies (#3795,
#3805)
- [l10n] Complete translations (with the help of translation tools)
(#3794)
- [refactor] Refactored `calendarfetcherutils` in Calendar module to
handle timezones better (#3806)
  - Removed as many of the date conversions as possible
- Use `moment-timezone` when calculating recurring events, this will fix
problems from the past with offsets and DST not being handled properly
- Added some tests to test the behavior of the refactored methods to
make sure the correct event dates are returned
- [linter] Enable ESLint rule `no-console` and replace `console` with
`Log` in some files (#3810)
- [tests] Review and refactor translation tests (#3792)

### Fixed

- [fix] Handle spellcheck issues (#3783)
- [calendar] fix fullday event rrule until with timezone offset (#3781)
- [feat] Add rule `no-undef` in config file validation to fix #3785
(#3786)
- [fonts] Fix `roboto.css` to avoid error message `Unknown descriptor
'var(' in @font-face rule.` in firefox console (#3787)
- [tests] Fix and refactor e2e test `Same keys` in
`translations_spec.js` (#3809)
- [tests] Fix e2e tests newsfeed and calendar to exit without open
handles (#3817)

### Updated

- [core] Update dependencies including electron to v36 (#3774, #3788,
#3811, #3804, #3815, #3823)
- [core] Update package type to `commonjs`
- [logger] Review factory code part: use `switch/case` instead of
`if/else if` (#3812)

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Michael Teeuw <[email protected]>
Co-authored-by: Kristjan ESPERANTO <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Ross Younger <[email protected]>
Co-authored-by: Veeck <[email protected]>
Co-authored-by: Bugsounet - Cédric <[email protected]>
Co-authored-by: jkriegshauser <[email protected]>
Co-authored-by: illimarkangur <[email protected]>
Co-authored-by: sam detweiler <[email protected]>
Co-authored-by: vppencilsharpener <[email protected]>
Co-authored-by: veeck <[email protected]>
Co-authored-by: Paranoid93 <[email protected]>
Co-authored-by: Brian O'Connor <[email protected]>
Co-authored-by: WallysWellies <[email protected]>
Co-authored-by: Jason Stieber <[email protected]>
Co-authored-by: jargordon <[email protected]>
Co-authored-by: Daniel <[email protected]>
Co-authored-by: Ryan Williams <[email protected]>
Co-authored-by: Panagiotis Skias <[email protected]>
Co-authored-by: Marc Landis <[email protected]>
Co-authored-by: HeikoGr <[email protected]>
Co-authored-by: Pedro Lamas <[email protected]>
Co-authored-by: veeck <[email protected]>
Co-authored-by: Magnus <[email protected]>
Co-authored-by: Ikko Eltociear Ashimine <[email protected]>
Co-authored-by: DevIncomin <[email protected]>
Co-authored-by: Nathan <[email protected]>
Co-authored-by: mixasgr <[email protected]>
Co-authored-by: Savvas Adamtziloglou <[email protected]>
Co-authored-by: Konstantinos <[email protected]>
Co-authored-by: OWL4C <[email protected]>
Co-authored-by: BugHaver <[email protected]>
Co-authored-by: BugHaver <[email protected]>
Co-authored-by: Koen Konst <[email protected]>
Co-authored-by: Koen Konst <[email protected]>
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.

5 participants