Skip to content

feat: support pnpm - #3351

Closed
easonshimmer wants to merge 14 commits into
electron:mainfrom
easonshimmer:feat/package-manager-support
Closed

feat: support pnpm#3351
easonshimmer wants to merge 14 commits into
electron:mainfrom
easonshimmer:feat/package-manager-support

Conversation

@easonshimmer

@easonshimmer easonshimmer commented Sep 14, 2023

Copy link
Copy Markdown
  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • The changes are appropriately documented (if applicable).
  • The changes have sufficient test coverage (if applicable).
  • The testsuite passes successfully on my local machine (if applicable).

Summarize your changes:

This PR supports projects to use Electron Forge while using pnpm as their package manager.

Related issue: #2633

Signed-off-by: Yi Yang <yangyi.makeryi@gmail.com>
@easonshimmer
easonshimmer requested a review from a team as a code owner September 14, 2023 14:42

@erickzhao erickzhao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hey @makeryi, thanks a lot for this contribution! Looks like the slow tests are failing at least because pnpm and bun are not installed in our CI configs.

@easonshimmer

Copy link
Copy Markdown
Author

Hey @makeryi, thanks a lot for this contribution! Looks like the slow tests are failing at least because pnpm and bun are not installed in our CI configs.

Thanks for your reply! I will look into the CI configs and try to make the tests pass.

Yi Yang added 3 commits September 15, 2023 07:34
Signed-off-by: Yi Yang <yangyi.makeryi@gmail.com>
…` functions

Signed-off-by: Yi Yang <yangyi.makeryi@gmail.com>
Signed-off-by: Yi Yang <yangyi.makeryi@gmail.com>
@erikian

erikian commented Sep 15, 2023

Copy link
Copy Markdown
Member

cc @BlackHole1

@dsanders11

Copy link
Copy Markdown
Member

I don't think installing bun and pnpm in CI is the right move here. Will the tests still locally pass without those installed?

@BlackHole1

BlackHole1 commented Sep 15, 2023

Copy link
Copy Markdown
Member

@makeryi Thank you for your PR!

I have also encountered a similar error in #3178. Maybe @caoxiemeihao can provide some suggestions.

I’m not very familiar with bun, so it might be better to split this PR into two separate PRs if possible.

@easonshimmer

easonshimmer commented Sep 15, 2023

Copy link
Copy Markdown
Author

I don't think installing bun and pnpm in CI is the right move here. Will the tests still locally pass without those installed?

Thanks for your reply!

I added bun and pnpm as NODE_INSTALLER in this test file packages/api/core/test/slow/api_spec_slow.ts, so i add bun and pnpm installaion in CI. Maybe it is a wrong way.

The tests still failed locally even with bun and pnpm installed.

  110 passing (17m)
  5 pending
  1 failing

  1) Electron Forge API
       after init
         after package
           make
             can make for the MAS platform successfully:
     RequestError: socket hang up
      at ClientRequest.<anonymous> (node_modules/got/dist/source/core/index.js:970:111)
      at Object.onceWrapper (node:events:629:26)
      at ClientRequest.emit (node:events:526:35)
      at ClientRequest.emit (node:domain:489:12)
      at ClientRequest.origin.emit (node_modules/@szmarczak/http-timer/dist/source/index.js:43:20)
      at TLSSocket.socketOnEnd (node:_http_client:525:9)
      at TLSSocket.emit (node:events:526:35)
      at TLSSocket.emit (node:domain:489:12)
      at endReadableNT (node:internal/streams/readable:1359:12)
      at connResetException (node:internal/errors:720:14)
      at TLSSocket.socketOnEnd (node:_http_client:525:23)
      at TLSSocket.emit (node:events:526:35)
      at TLSSocket.emit (node:domain:489:12)
      at endReadableNT (node:internal/streams/readable:1359:12)
      at processTicksAndRejections (node:internal/process/task_queues:82:21)



Failed with exit code: 1
Output:

error Command failed with exit code 255.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@easonshimmer

Copy link
Copy Markdown
Author

@makeryi Thank you for your PR!

I have also encountered a similar error in #3178. Maybe @caoxiemeihao can provide some suggestions.

I’m not very familiar with bun, so it might be better to split this PR into two separate PRs if possible.

Thanks for your context!

I am not familiar with bun too. Node and Bun are two different runtimes, maybe split this PR into two separate PRs is better than now. If so, i will make this PR to support pnpm only and create a new PR to support bun if i can do that.

Signed-off-by: Yi Yang <yangyi.makeryi@gmail.com>
@easonshimmer easonshimmer changed the title feat: support pnpm and bun feat: support pnpm Sep 15, 2023
@BlackHole1 BlackHole1 self-assigned this Sep 15, 2023
@caoxiemeihao

Copy link
Copy Markdown
Member

I have also encountered a similar error in #3178. Maybe @caoxiemeihao can provide some suggestions.

Haha! The slow test error from #3178 only appears in Windows machine. 😅

@erickzhao erickzhao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for this!

Comment thread packages/utils/core-utils/src/package-manager.ts Outdated
Comment thread packages/api/cli/src/util/check-system.ts Outdated
Comment thread packages/api/cli/src/util/check-system.ts Outdated
Comment thread packages/api/core/src/util/install-dependencies.ts Outdated
Comment thread packages/api/core/src/util/install-dependencies.ts Outdated
Comment thread packages/utils/core-utils/test/package-manager_spec.ts Outdated
Yi Yang added 5 commits September 20, 2023 08:32
Signed-off-by: Yi Yang <yangyi.makeryi@gmail.com>
Signed-off-by: Yi Yang <yangyi.makeryi@gmail.com>
Signed-off-by: Yi Yang <yangyi.makeryi@gmail.com>
Signed-off-by: Yi Yang <yangyi.makeryi@gmail.com>
Signed-off-by: Yi Yang <yangyi.makeryi@gmail.com>
@easonshimmer

Copy link
Copy Markdown
Author

Finally all tests passed (both in my local machine and CI)

@erickzhao erickzhao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hey @makeryi, thanks for the quick turnaround time for addressing my feedback! One more thing I wanted to say is that I liked the npm_config_user_agent approach by default (didn't know that var existed before you raised this PR). I think NODE_INSTALLER should be an override for it with npm_config_user_agent being used by default. 🤔

…ting

Signed-off-by: Yi Yang <yangyi.makeryi@gmail.com>
@easonshimmer

Copy link
Copy Markdown
Author

Hey @makeryi, thanks for the quick turnaround time for addressing my feedback! One more thing I wanted to say is that I liked the npm_config_user_agent approach by default (didn't know that var existed before you raised this PR). I think NODE_INSTALLER should be an override for it with npm_config_user_agent being used by default. 🤔

Thanks for your suggestion!
I did it and added real user agent data for testing the npm_config_user_agent env var.

@TranquilMarmot

Copy link
Copy Markdown

Any chance of this getting merged soon? Need somebody to try it out?

@BlackHole1

Copy link
Copy Markdown
Member

@TranquilMarmot I just came back from vacation, and recently I will review this PR.

Comment thread package.json Outdated
Comment thread packages/utils/core-utils/src/package-manager.ts Outdated
Comment thread packages/utils/core-utils/src/package-manager.ts Outdated
Comment thread packages/utils/core-utils/src/package-manager.ts Outdated
Comment thread packages/utils/core-utils/src/package-manager.ts Outdated
@TobiasMouritzen

Copy link
Copy Markdown

Can I do something to help this merge? I would very much like to use electron-forge in my pnpm monorepo.

Yi Yang added 2 commits October 19, 2023 22:46
Signed-off-by: Yi Yang <yangyi.makeryi@gmail.com>
Signed-off-by: Yi Yang <yangyi.makeryi@gmail.com>
@easonshimmer

Copy link
Copy Markdown
Author

Sorry for the late reply.
I think everything works fine now.

@aogg

aogg commented Oct 22, 2023

Copy link
Copy Markdown

pnpm create electron-app@latest hid-electron --template=vite

It doesn't work.

@easonshimmer easonshimmer closed this by deleting the head repository Oct 29, 2023
@seahindeniz

Copy link
Copy Markdown

Hi @makeryi, the PR is closed. Unintentional action perhaps? 🤔

@erickzhao

Copy link
Copy Markdown
Member

I can take this up and reland it if @makeryi is no longer interested. 🥺

@GitMurf GitMurf mentioned this pull request Nov 9, 2023
3 tasks
@artrixdotdev

Copy link
Copy Markdown

any updates?

@akash07k

Copy link
Copy Markdown

@makeryi Have you discontinued the work on it? asking because the PR is closed.

@goosewobbler goosewobbler mentioned this pull request Apr 24, 2024
5 tasks
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.