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 production postinstall #35

Merged
merged 3 commits into from
Oct 17, 2023
Merged

fix production postinstall #35

merged 3 commits into from
Oct 17, 2023

Conversation

dd137
Copy link

@dd137 dd137 commented Oct 17, 2023

As noted here, adding the meteor-desktop package to an npm project produces a sh: babel: command not found error. Full log:

$ npm i --save-dev @meteor-community/meteor-desktop@preview

> @meteor-community/[email protected] postinstall /.../proj/node_modules/@meteor-community/meteor-desktop
> npm run build && node dist/scripts/addToScripts || echo


> @meteor-community/[email protected] build /.../proj/node_modules/@meteor-community/meteor-desktop
> babel lib --out-dir dist --source-maps inline --copy-files

sh: babel: command not found
npm ERR! code ELIFECYCLE
npm ERR! syscall spawn
npm ERR! file sh
npm ERR! errno ENOENT
npm ERR! @meteor-community/[email protected] build: `babel lib --out-dir dist --source-maps inline --copy-files`
npm ERR! spawn ENOENT
npm ERR!
npm ERR! Failed at the @meteor-community/[email protected] build script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
npm WARN Local package.json exists, but node_modules missing, did you mean to install?

npm ERR! A complete log of this run can be found in:
npm ERR!     /.../.npm/_logs/2023-10-17T10_11_11_853Z-debug.log

npm WARN @meteor-community/[email protected] requires a peer of app-builder-lib@* but none is installed. You must install peer dependencies yourself.
npm WARN @meteor-community/[email protected] requires a peer of electron-builder@* but none is installed. You must install peer dependencies yourself.
npm WARN @meteor-community/[email protected] requires a peer of electron-packager@* but none is installed. You must install peer dependencies yourself.
npm WARN [email protected] No description
npm WARN [email protected] No repository field.

+ @meteor-community/[email protected]
added 311 packages from 107 contributors and audited 311 packages in 16.57s

24 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities

commit 1:

While this doesn't prevent the installation of the package, it is annoying. This happens since 3.1.1-rc.0. I believe it is due to 1d04a58 which added npm run build to the postinstall script. I don't think this is correct – build should run before publishing or after changes in dev, not after a normal package installation. I propose to revert this change.

commit 2:

postinstall's npm run build allowed addToScripts.js to run even after a simple npm i inside meteor-desktop/ (for dev purpose). But as far as I can see, it doesn't make sense to run addToScripts.js in such cases, because if we're not adding meteor-desktop to another project, there's no package.json addToScripts.js needs to modify.

That being said, a side benefit was also to prevent postinstall from logging a MODULE_NOT_FOUND error when running npm i inside meteor-desktop/ for the first time. So to keep that behavior while avoiding run builds in normal package installs, the 2nd commit simply checks for addToScripts.js's existence before running it (at the cost of an additional prod dependency).

dd137 added 3 commits October 17, 2023 11:57
Reverts 1d04a5.
`build` requires @babel/cli, which is a dev dependency, so meteor-desktop install triggers errors in production.
When doing `npm i` inside meteor-desktop/ for the first time, dist/scripts/addToScripts doesn't exist. That doesn't matter as in such case, there's no package.json to which it would make sense to add the "desktop" script. Checking whether addToScripts exists first eliminates the useless error from the installation process.
@dd137 dd137 marked this pull request as ready for review October 17, 2023 13:42
@dd137 dd137 requested a review from StorytellerCZ October 17, 2023 13:43
@dd137
Copy link
Author

dd137 commented Oct 17, 2023

@StorytellerCZ Could you please publish a rc.2 so that we can test whether the normal package installation now works correctly?

As far as I'm concerned, if this issue is fixed, we can publish 3.1.1. Besides testing with skeleton projects, I successfully migrated our main project to 3.1.1-rc.0 and things seem to work properly. More tests are always welcome of course (especially on Windows).

@a4xrbj1
Copy link

a4xrbj1 commented Oct 17, 2023

sh: babel: command not found

I'm getting the same error as you @dd137 when I upgrade to version 3.1.1-rc.1

I've went ahead and started building my Electron app locally but got the following error:

ERROR  electronApp:  error while merging dependencies list:  Error: While processing dependencies from module[requests], a dependency request: 2.88.0 was found to be conflicting with a dependency (2.88.2) that was already declared in other module or it is used in core of the electron app.
    at forEach (/Users/andreaswest/Documents/workspace/frontend/node_modules/@meteor-community/meteor-desktop/lib/dependenciesManager.js:183:27)
    at Array.forEach (<anonymous>)
    at DependenciesManager.detectDuplicatedDependencies (/Users/andreaswest/Documents/workspace/frontend/node_modules/@meteor-community/meteor-desktop/lib/dependenciesManager.js:181:24)
    at DependenciesManager.mergeDependencies (/Users/andreaswest/Documents/workspace/frontend/node_modules/@meteor-community/meteor-desktop/lib/dependenciesManager.js:125:18)
    at forEach (/Users/andreaswest/Documents/workspace/frontend/node_modules/@meteor-community/meteor-desktop/lib/electronApp.js:694:30)
    at Array.forEach (<anonymous>)
    at ElectronApp.updateDependenciesList (/Users/andreaswest/Documents/workspace/frontend/node_modules/@meteor-community/meteor-desktop/lib/electronApp.js:693:50)
    at ElectronApp.build (/Users/andreaswest/Documents/workspace/frontend/node_modules/@meteor-community/meteor-desktop/lib/electronApp.js:353:18)
    at MeteorDesktop.run (/Users/andreaswest/Documents/workspace/frontend/node_modules/@meteor-community/meteor-desktop/lib/index.js:123:9)

Any idea if this is related to the error you posted or is this a new one? The request package is at version:

"request": "^2.88.2"

for a while now, without any problems. I've just built a new version yesterday and my users aren't reporting any problems.

@StorytellerCZ
Copy link
Member

@dd137 I'm heading out for a bit. Will merge and publish once I get back.

@StorytellerCZ StorytellerCZ merged commit 815e422 into fix/bug-12 Oct 17, 2023
3 checks passed
@StorytellerCZ StorytellerCZ deleted the fix/postinstall branch October 17, 2023 19:24
@dd137
Copy link
Author

dd137 commented Oct 18, 2023

I'm getting the same error as you @dd137 when I upgrade to version 3.1.1-rc.1

Seems to be fixed now with rc.2!

ERROR electronApp: error while merging dependencies list: Error: While processing dependencies from module[requests], a dependency request: 2.88.0 was found to be conflicting with a dependency (2.88.2) that was already declared in other module or it is used in core of the electron app.

Any idea if this is related to the error you posted or is this a new one?

This is unrelated. Do you have a "request": "2.88.0" dependency somewhere in your desktop code (check .desktop/settings.json and .desktop/**/module.jsons)? If so you'll have to update it to 2.88.2, to match meteor-desktop's version (updated to 2.88.2 since 3.1.1-rc.0 I think).

By the way, I noticed that the yarn.lock file is not up to date (it references request 2.88.0). Should we keep it up-to-date as well @StorytellerCZ? (I don't use yarn)

@a4xrbj1
Copy link

a4xrbj1 commented Oct 18, 2023

This is unrelated. Do you have a "request": "2.88.0" dependency somewhere in your desktop code (check .desktop/settings.json and .desktop/**/module.jsons)? If so you'll have to update it to 2.88.2, to match meteor-desktop's version (updated to 2.88.2 since 3.1.1-rc.0 I think).

Yes, it's coming from the .desktop/modules/request/modules.json file which has a reference to 2.88.0 in it.

Changing it to 2.88.2 solved the error and the app is now starting with the newest preview version locally. Thanks @dd137 and @StorytellerCZ

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.

3 participants