Page MenuHomePhabricator
Differential D5735

[desktop] Add babel support
ClosedPublic
Actions

Authored by michal on Nov 28 2022, 6:59 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 9, 11:50 AM
Unknown Object (File)
Sat, Nov 9, 11:50 AM
Unknown Object (File)
Sat, Nov 9, 11:50 AM
Unknown Object (File)
Sat, Nov 9, 11:50 AM
Unknown Object (File)
Sat, Nov 9, 11:50 AM
Unknown Object (File)
Sat, Nov 9, 11:50 AM
Unknown Object (File)
Sat, Nov 9, 11:50 AM
Unknown Object (File)
Sat, Nov 9, 11:49 AM
View All Files
Subscribers
ashoat

Details

Reviewers
tomek
atul
ashoat
Commits
rCOMM4515286fad38: [desktop] Add babel support
Summary

Manually calls babel and transforms files. First step towards webpack support.

It currently transforms all files in src/ and moves them to dist/. This means that absolute paths pointing inside src/ won't work which isn't ideal will be fixed in the future when webpack will be working.

Test Plan
  • Test if development version works (yarn dev)
  • Test if prod wersion works (yarn package)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

michal created this revision.Nov 28 2022, 6:59 AM
Herald added a subscriber: ashoat. · View Herald TranscriptNov 28 2022, 6:59 AM
michal updated this revision to Diff 18873.Nov 28 2022, 7:00 AM

Add a newline

Harbormaster returned this revision to the author for changes because remote builds failed.Nov 28 2022, 7:00 AM
Harbormaster failed remote builds in B13786: Diff 18872!
Harbormaster failed remote builds in B13787: Diff 18873!
michal added inline comments.Nov 28 2022, 7:14 AM
desktop/forge.config.cjs
32 ↗(On Diff #18873)

electron complains if we pass it an ESM file, and the root package.json contains type: 'commonjs'. Because of it package.json is added to dist/ to treat the .js files in dist/ as commonjs.

We could instead rename the files to .cjs but then the relative paths (e.g. ./preload.js) would break.

37–49 ↗(On Diff #18873)

startLogic runs only in development, just before the application starts.
prePackage hook runs during the packaging of the final executable.

55 ↗(On Diff #18873)

These files won't be packaged into the final executable (these are regex patterns and not globs)

desktop/package.json
2–6 ↗(On Diff #18873)

electron-packager prunes devDependencies from the final executable. Unfortunately, it seems like it can't handle yarn workspaces. nohoist makes it so all dependencies of desktop will be included in the desktop/node_modules and not in the root node_modules. This wasn't needed before, because all desktop dependencies were electron specific.

More info here

michal updated this revision to Diff 19133.Dec 5 2022, 3:49 AM

Try to fix the CI build

Harbormaster completed remote builds in B13964: Diff 19133.Dec 5 2022, 4:02 AM
michal requested review of this revision.Dec 5 2022, 4:02 AM
ashoat requested changes to this revision.Dec 5 2022, 6:21 AM
ashoat added inline comments.
.eslintignore
24–25 ↗(On Diff #19133)

Should these be in the same order as they appear in .gitignore?

desktop/babel.config.cjs
3 ↗(On Diff #19133)

What happens if we take modules: 'commonjs' out?

desktop/forge.config.cjs
1 ↗(On Diff #19133)

Why can't forge.config.cjs be ESM? Who calls it? As mentioned below I'm confused about the relationship between electron-forge and electron

32 ↗(On Diff #19133)

electron complains if we pass it an ESM file, and the root package.json contains type: 'commonjs'. Because of it package.json is added to dist/ to treat the .js files in dist/ as commonjs.

We could instead rename the files to .cjs but then the relative paths (e.g. ./preload.js) would break.

  1. The root package.json appears to have type: 'module'... is that what you meant?
  2. I'm confused by what you mean by "pass it an ESM file"... what files are being passed to Electron? Is this forge.config.cjs being passed to Electron? Or are you talking about the files in the dist folder? Can you remind me again why those files can't just be ESM? Does Electron basically say "I only work with CJS, and thus I refuse to work if you specify type: 'module'?
37–49 ↗(On Diff #19133)

startLogic runs only in development, just before the application starts.
prePackage hook runs during the packaging of the final executable.

Can you add code comments explaining this?

46 ↗(On Diff #19133)

Can you add a bit more context in this code comment? I don't understand the relationship between electron-forge and Electron (both in dev vs. prod mode)

desktop/package.json
2–6 ↗(On Diff #19133)

electron-packager prunes devDependencies from the final executable. Unfortunately, it seems like it can't handle yarn workspaces. nohoist makes it so all dependencies of desktop will be included in the desktop/node_modules and not in the root node_modules. This wasn't needed before, because all desktop dependencies were electron specific.

More info here

Can you link something that explains that electron-packager works this way? A great option would be a GitHub issue tracking the problem. A second option could be an example repo that uses Electron with Yarn Workspaces and needs to use nohoist (I found this).

This revision now requires changes to proceed.Dec 5 2022, 6:21 AM
michal added a comment.EditedDec 6 2022, 5:29 AM

electron-forge is a package that handles building, packaging, and code signing. It uses different packages for this (e.g. electron-packager, osx-sign) and provides a unified configuration for them. It's not included in the final application. When you run electron-forge package or some other command it uses forge.config.cjs for configuration.

When you run the Comm app, the electron executable is run on the dist/main.js file (because it's specified in the main field in the package.json).

Does Electron basically say "I only work with CJS, and thus I refuse to work if you specify type: 'module'"

Yeah, basically. There's a big open issue on this topic.

Similarly electron-forge also doesn't support ESM for the config file, and throws an error.

desktop/babel.config.cjs
3 ↗(On Diff #19133)

According to the documentation if we don't specify the modules option it will look for the caller and decide based on that. The caller option is most often set by the bundler plugin (like babel-loader), but we are currently using babel directly so it's not set.

If we remove the modules: 'commonjs' (and don't set caller) it seems to still convert the modules to cjs, but I don't think this is specified in the documentation.

So we could:

  • remove modules: 'commonjs'
  • leave the modules: 'commonjs' (and remove it when we integrate webpack)
  • remove modules: 'commonjs' and specify the caller option (and remove caller when we integrate webpack)
desktop/package.json
2–6 ↗(On Diff #19133)

Here's an an github tracking this issue for some other repository: link.

In particular:

Workspaces and electron-forge don't seem to play especially nice together. Issues about when googling those topics, the seminal one seeming to be: electron/forge#869 The authors say it was fixed in electron-forge v6; that seems wrong. We use v6.0.3, and consistently get:

Error: Cannot find the package "electron". Perhaps you need to run "yarn install"

Unless applying the nohoist option recommended in this comment electron/forge#869

The links are to the electron-forge issue about this problem. Also you can see that it this repo they are specyf

michal added a comment.Dec 6 2022, 5:36 AM

Sorry, I sent the comments by mistake before finishing the inline comment about nohoist and I don't think I can edit it.

What I didn't add was that in the linked repo they are using nohoist only with some specific packages, but in our case I need to include all packages (**), otherwise I'm still getting errors.

michal updated this revision to Diff 19173.Dec 6 2022, 5:45 AM

Add comments, change order in .eslintignore

Harbormaster completed remote builds in B14000: Diff 19173.Dec 6 2022, 5:57 AM
michal added a child revision: D5825: [desktop] Code signing for disk image.Dec 6 2022, 6:37 AM
michal updated this revision to Diff 19183.Dec 6 2022, 6:42 AM

Small changes: add name to the BabelPlugin (it's more clearer what's happening in the stdout during packaging) and simplify ignore pattern to ignore whole folder.

Harbormaster completed remote builds in B14010: Diff 19183.Dec 6 2022, 6:54 AM
ashoat accepted this revision.Dec 6 2022, 9:25 AM

Thanks for explaining everything!! I think this makes sense now. Regarding modules: 'commonjs' in babel.config.cjs, I think it makes sense to be explicit there. If that is referring to Babel's output that makes sense. If it refers to the input it should be changed

This revision is now accepted and ready to land.Dec 6 2022, 9:25 AM
ashoat added 1 blocking reviewer(s): tomek.Dec 6 2022, 9:26 AM

Would love if @tomek could take a look as well

This revision now requires review to proceed.Dec 6 2022, 9:26 AM
tomek accepted this revision.Dec 7 2022, 4:46 AM
tomek added inline comments.
desktop/babel.config.cjs
3 ↗(On Diff #19183)

Guess we can keep this version but it seems like 22 was released recently

desktop/forge.config.cjs
51 ↗(On Diff #19183)

Is this a typo?

desktop/package.json
23–36 ↗(On Diff #19183)

How were the versions chosen? Are they the most recent, or consistent with other places?

This revision is now accepted and ready to land.Dec 7 2022, 4:46 AM
michal updated this revision to Diff 19207.Dec 7 2022, 5:41 AM

Fix typo and update versions

desktop/package.json
23–36 ↗(On Diff #19183)

The @babel/* packages are consistent with other workspaces.

Harbormaster completed remote builds in B14032: Diff 19207.Dec 7 2022, 5:52 AM
Closed by commit rCOMM4515286fad38: [desktop] Add babel support (authored by Michal Gniadek <michal.gniadek@swmansion.com>). · Explain WhyDec 12 2022, 2:39 AM
This revision was automatically updated to reflect the committed changes.
Michal Gniadek <michal.gniadek@swmansion.com> added a commit: rCOMM4515286fad38: [desktop] Add babel support.

Revision Contents
Changeset List

PathSize
2 lines
1 line
desktop/
12 lines
61 lines
33 lines
src/
main.js
main.cjs
16 lines
preload.js
preload.cjs
2 lines
234 lines

Diff 19285

View Options

.eslintignore

Loading...
View Options

.gitignore

Loading...
View Options

desktop/babel.config.cjs

Loading...
View Options

desktop/forge.config.cjs

Loading...
View Options

desktop/main.cjs

Loading...
View Options

desktop/package.json

Loading...
View Options

desktop/preload.cjs

Loading...
View Options

desktop/src/main.js

Loading...
View Options

desktop/src/preload.js

Loading...
View Options

yarn.lock

Loading...