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

feat: move create-electron-app into forge #2988

Merged
merged 22 commits into from Nov 1, 2022
Merged

Conversation

VerteDinde
Copy link
Member

@VerteDinde VerteDinde commented Oct 25, 2022

This PR moves create-electron-app into Forge's monorepo:

  • Makes create-electron-app TS compatible
  • Changes the version to match the other monorepo packages
  • electron-userland should have create-electron-app NPM publish permissions

@codecov
Copy link

codecov bot commented Oct 25, 2022

Codecov Report

Merging #2988 (359d616) into main (36dcdd0) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2988   +/-   ##
=======================================
  Coverage   73.80%   73.80%           
=======================================
  Files          67       67           
  Lines        2168     2168           
  Branches      436      436           
=======================================
  Hits         1600     1600           
  Misses        361      361           
  Partials      207      207           

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 36dcdd0...359d616. Read the comment docs.

@VerteDinde VerteDinde marked this pull request as ready for review October 26, 2022 20:01
.eslintignore Outdated Show resolved Hide resolved
@VerteDinde VerteDinde force-pushed the move-create-electron-app branch 2 times, most recently from 77274e0 to 1002b31 Compare October 26, 2022 23:13
@VerteDinde
Copy link
Member Author

VerteDinde commented Nov 1, 2022

I'm fairly certain this is a bug in Bolt's Windows symlinking, and this method specifically is where I think we're experiencing the problem: https://github.com/boltpkg/bolt/blob/a922d937c8054dea4c4e1d4f8755583187cdd9ed/src/utils/fs.js#L59

The new package works as expected on Mac and Linux; it's only failing on Windows, but it's failing both in CI and when I run bolt locally after running git clean -dfx. Bolt tries to reconcile Windows symlinks differently than Posix ( see here ), and one of the ways it attempts to do that is by falling back to the file in src if the filepath or symlink does not exist ( see here ).

In our case, create-electron-app is looking for cli/dist/electron-forge.js - that file won't exist until the typescript compiler is run. Trying to fall back to src will only show a ts file, which Bolt doesn't think is what we need.

Not 100% sure how to get around this yet, but I think this is the only blocker for this PR.

@VerteDinde VerteDinde force-pushed the move-create-electron-app branch 2 times, most recently from 7bea467 to a58d533 Compare November 1, 2022 14:51
@VerteDinde
Copy link
Member Author

Don't merge, please, want to clean this up a bit now that it's working

@VerteDinde VerteDinde marked this pull request as draft November 1, 2022 18:01
@VerteDinde VerteDinde force-pushed the move-create-electron-app branch 3 times, most recently from d05eb38 to 8770d7b Compare November 1, 2022 20:38
tools/maybe-shim-windows.js Outdated Show resolved Hide resolved
tools/maybe-shim-windows.js Outdated Show resolved Hide resolved
@MarshallOfSound
Copy link
Member

I think the mkdirp and touch deps can be removed now too

@VerteDinde VerteDinde marked this pull request as ready for review November 1, 2022 21:10
Co-authored-by: Samuel Attard <sam@electronjs.org>
@VerteDinde VerteDinde requested review from MarshallOfSound and a team November 1, 2022 21:55
@VerteDinde VerteDinde merged commit a2eadbc into main Nov 1, 2022
@VerteDinde VerteDinde deleted the move-create-electron-app branch November 1, 2022 22:27
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.

None yet

4 participants