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: vscode debugging shell file #2908

Closed
wants to merge 2 commits into from

Conversation

wesyao
Copy link

@wesyao wesyao commented Jul 12, 2022

  • 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:
The debugging experience is broken on mac/linux and was introduced #535.

There has been a request for this issue #1369.

@codecov
Copy link

codecov bot commented Jul 12, 2022

Codecov Report

Merging #2908 (9d7d4a0) into master (345250c) will decrease coverage by 0.12%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2908      +/-   ##
==========================================
- Coverage   71.82%   71.69%   -0.13%     
==========================================
  Files          79       79              
  Lines        2385     2392       +7     
  Branches      449      450       +1     
==========================================
+ Hits         1713     1715       +2     
- Misses        450      548      +98     
+ Partials      222      129      -93     
Impacted Files Coverage Δ
packages/utils/async-ora/src/ora-handler.ts 81.81% <0.00%> (-4.85%) ⬇️
packages/api/core/src/api/start.ts 64.04% <0.00%> (-1.84%) ⬇️
packages/maker/base/src/Maker.ts 69.69% <0.00%> (ø)
packages/api/core/src/api/make.ts 79.00% <0.00%> (ø)
packages/api/core/src/util/hook.ts 75.00% <0.00%> (ø)
packages/api/core/src/api/import.ts 61.76% <0.00%> (ø)
packages/api/core/src/api/package.ts 75.55% <0.00%> (ø)
packages/api/core/src/api/publish.ts 68.42% <0.00%> (ø)
packages/api/core/src/util/out-dir.ts 66.66% <0.00%> (ø)
packages/api/core/src/util/parse-archs.ts 60.00% <0.00%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 345250c...9d7d4a0. Read the comment docs.

Copy link
Member

@VerteDinde VerteDinde left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@malept
Copy link
Member

malept commented Jul 13, 2022

It would be good if someone could test the following scenarios before merging:

  • standalone git repo on Linux
  • standalone git repo on macOS
  • monorepo on Linux
  • monorepo on macOS

@malept
Copy link
Member

malept commented Jul 13, 2022

Additionally the PR title does not conform to conventional commits standards as per the contributing docs

Copy link
Member

@malept malept left a comment

Choose a reason for hiding this comment

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

Holding on this based on my comments

@VerteDinde VerteDinde changed the title fix vscode debugging shell file fix: vscode debugging shell file Jul 13, 2022
Copy link
Member

@VerteDinde VerteDinde left a comment

Choose a reason for hiding this comment

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

Agreed with @malept on the catches 🙇‍♀️ @wesyao Let us know if you need these changes tested on any of the above platforms, happy to test on Ubuntu if needed

@wesyao
Copy link
Author

wesyao commented Jul 13, 2022

Thanks for taking a look @malept & @VerteDinde.

Can you help me define a test plan @VerteDinde? I want to make sure we're testing the exact same way to ensure parity. Also, if you could help me with Ubuntu, I'd greatly appreciate it (no linux environment for me). 😅

@malept , what differences are you seeing between a mono repo and a stand alone git repo?

@malept
Copy link
Member

malept commented Jul 13, 2022

The node_modules folder exists in different places depending on the monorepo manager.

@wesyao
Copy link
Author

wesyao commented Jul 13, 2022

So, you'd just like me to simulate a repo with various directories and the node_modules directory in some subfolder?

Something like this?

- Repo
|__folder 1
|__|__folder a
|__|__|__app
|__|__|__node_modules

I wanted to also point out that I'm really just reverting it back before this PR: #536

@malept
Copy link
Member

malept commented Jul 13, 2022

I don't think simulation is very realistic, by definition. Using actual monorepo managers is preferred.

@wesyao
Copy link
Author

wesyao commented Jul 14, 2022

Hey @VerteDinde, I followed up with @malept to see what specifically needs to be tested to verify this change. I don't have an Ubuntu environment. If you could help me test the Ubuntu flow, I'd appreciate it! 😄

Here is the test matrix that needs to be tested:
pnpm, npm workspaces, yarn workspaces, lerna

Just to be transparent, this experience is currently broken and it is a documented as being supported here.

I'll tackle the macos experience and circle back.

Thanks to all!

@VerteDinde
Copy link
Member

VerteDinde commented Jul 15, 2022

@wesyao Sure thing! I'll give these a test on Ubuntu next week (PST), and will report back here 🙂

@wesyao wesyao requested a review from a team as a code owner February 7, 2024 18:18
@dsanders11
Copy link
Member

This should be resolved by electron-forge/electron-forge-docs#131, so closing this out - if there's still a problem here, feel free to ping me and I'll re-open.

@dsanders11 dsanders11 closed this Feb 7, 2024
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.

4 participants