thombruce / toodles

✅ A super simple todo app
https://toodles.thombruce.com/
GNU General Public License v3.0
0 stars 0 forks source link

[Feature]: Get Electron builds up and running #120

Closed thombruce closed 1 month ago

thombruce commented 1 month ago

Feature request

In cross-development with @thombruce/tnt-electron, ensure that Electron projects can actually be built...

Right now they run in dev, but do not build for production.

I do need to change the package.json such that the build script actually calls electron-builder (or Electron Forge if we do decide that's worth tagging in instead), e.g.

  "scripts": {
    "electron:build": "ELECTRON=true nuxt build --prerender && electron-builder"
  }

However I have already done this and I am faced with an error during the build. Even when I explicitly add electron to my package.json, I get an error resembling:

⨯ Cannot compute electron version from installed node modules - none of the possible electron modules are installed and version ("^31.3.1") is not fixed in project.
See https://github.com/electron-userland/electron-builder/issues/3984#issuecomment-504968246

How do I "fix" the version in my project? I don't know.

Beyond this, there are dependency issues seen in GitHub Actions that were not present during a similar build on my machine - these may crop up again when we get closer to fixing the above.

Recommend we do get Linux builds working on my machine first of all, then... then see if those errors continue to crop up in GitHub actions. Squash them as they do. After this, we can think about publishing to release tags... which means if I'm not already, I need to start tagging releases on Toodles (probably making use of Lerna for this). Ah, I'm reminded, we definitely already do tag releases on Toodles using Lerna - I just haven't done so in a minute because the Electron package is not strictly ready for release (clearly!).

Note: Given that the error I'm experiencing above is an electron-builder one, maybe it is a good idea to give Forge a go and see if it gets past that step. The two packages have... slightly different end goals, maybe slightly different build targets? But what I'm concerned with is: Can I build for Windows, Mac and Linux? And I'm pretty sure they both do do that. One (electron-builder, I think) just has additional support for, say, Debian packages? Worth digging into anyway.

Code of Conduct

thombruce commented 1 month ago

not fixed in workspace

This seems to mean that I should list a specific version of electron in my package.json.

- "electron": "^31.3.1",
+ "electron": "31.3.1", 

There's a chance I also need to add a nohoist rule to my root package.json:

-   "workspaces": [
-     "packages/*"
-   ],
+   "workspaces": {
+     "packages": ["packages/*"],
+     "nohoist": ["electron"]
+   },

Testing this now. I've ran a successful build from the electron directory both with and without the nohoist rule...

Hmm...

So I think I can omit it. Maybe the electron developers caught up to the hoisting mechanism and this is now appropriately supported. See Yarn's blog post about this dated 2018: https://classic.yarnpkg.com/blog/2018/02/15/nohoist/

Since it works without (for now), I will leave that change out of the mix.

thombruce commented 1 month ago

There are some warnings during the (successful) app build:

So we need to add a description, author, icon and category (per build target, maybe).

thombruce commented 1 month ago

I've added author and description to package.json. Need to look into where the icon and app category should be set and what options are valid.


We still get an error when building on GitHub Actions:

Error:  Nuxt Build Error: [vite]: Rollup failed to resolve import "@thombruce/tnt/composables/tntApi" from "/home/runner/work/toodles/toodles/packages/app/stores/todos.ts".
  1. tntApi is a composable in my core layer. It should be auto-imported - the reason I tried importing it directly (and successfully did so) is because the Typescript document complains about it being undefined otherwise. But I can try omitting the import line and see if actually... it is still automatically imported?
  2. Alternatively, moving @thombruce/tnt-electron to dependencies (rather than devDependencies) might resolve the issue.
  3. Alternatively, alternatively... maybe my module(s) are being hoisted and I should add that nohoist rule for them.
  4. And if none of that works... well, I'm stumped and more digging will be required.
thombruce commented 1 month ago

Windows builds are hanging. Latest has been running for 34 minutes. Previous jobs have failed after... up to 6 hours.

This one failed after about five minutes: https://github.com/thombruce/toodles/actions/runs/10339954159/job/28619950265

Those jobs which do run for six hours are being automatically cancelled. Fair.

There are some issues reported with unrelated packages in the install dependencies step - changing to cd into relevant directory to see if this resolves/ignores those irrelevant issues. Could be there's an issue running yarn on Windows? Could change command to npm if this might help.

Seems to be an issue with the Windows environment anyway - my configuration for the job is not at all sophisticated or edge-casey:

  build_on_win:
    runs-on: windows-latest
    steps:
    - uses: actions/checkout@v4
    - uses: actions/setup-node@master
      with:
        node-version: 20
    - name: install dependencies
      run: cd packages/app && yarn install
    - name: build
      run: cd packages/app && yarn run electron:build
thombruce commented 1 month ago

As hypothesised, it does appear to be a Yarn issue: https://github.com/yarnpkg/yarn/issues/9004 <- Found this when searching for the error text with "windows" and "GitHub Actions" keywords, but I made no mention of Yarn.

So... NPM or there are a few other options suggested here: https://github.com/yarnpkg/yarn/issues/9004#issuecomment-1803643905

Those workarounds are work for me.

This one is faster than the others

RUN yarn install --network-timeout 1000000 My project doesn't have a lock file so npm install works for me.

RUN npm install Also, this could works

RUN yarn cache clean RUN yarn config delete proxy RUN yarn config delete https-proxy RUN yarn config delete registry RUN yarn install --network-timeout 1000000

Since I do have a lockfile, we'll try passing that network-timeout argument first... Is this extending the time for an expected response? That could make sense as a solution/root of the problem if, for some reason, network requests are slower on the Windows virtual machine.

The third suggestion also makes use of that argument but also... cleans the cache, deletes the proxy and registry and then runs with that argument. We'll try that second if the first option does not work.

thombruce commented 1 month ago

First attempt with the new argument does not yield the observed timeout error, so seems to be a viable solution. We still get complaints about irrelevant packages, however... so maybe adding the cache clean is a good idea.

My next push is going to add a working-directory config at the job level so that I don't have to write cd packages/app into each run step. I'll also go ahead and add those clean/delete steps to see if that clears out the irrelevant details; might remove these in a future push.

thombruce commented 1 month ago

Still watching the action for the previous push. Windows has gotten past the install dependencies step but is still working on the build step...

Has been for five minutes where other platforms take about 2.

Not necessarily a sign that it is broken, but if it goes on too long... let's say more than half an hour without success... then we're going to have to try something else.

thombruce commented 1 month ago

Latest job did not remove irrelevant warning: warning toodles@0.0.12: The engine "vscode" appears to be invalid.

I'm just going to remove those unneeded cache cleaning steps then - they didn't do anything useful and we'll keep the file as simple as we can until we need to.

thombruce commented 1 month ago

This is the job to watch right now: https://github.com/thombruce/toodles/actions/runs/10340621196

Though I suspect the build step is still hanging for some reason.

These are our steps:

    steps:
    - uses: actions/checkout@v4
    - uses: actions/setup-node@master
      with:
        node-version: 20
    - name: install dependencies
      run: yarn install --network-timeout 1000000
    - name: build
      run: yarn run electron:build

There's nothing special about that.

Could it be a Node version issue? Could it be... something wrong with our yarn run electron:build command.? Here's what that looks like:

"electron:build": "ELECTRON=true nuxt build --prerender && electron-builder",

Maybe the environment variable? It's necessary to build our Nuxt app for the electron environment, but maybe it's invalid for Windows in that form?

Oh yeah, that's EXACTLY the issue: https://stackoverflow.com/a/27090755

Okay, so options (these are just copied straight from the SO comments and will need to be modified, but illustrate the solutions)...

  1. set NODE_ENV=test&& mocha --reporter spec < This is not platform agnostic - it will be a Windows only solution
  2. env NODE_ENV=test mocha --reporter spec < This is platform agnostic, allegedly, but there are caveats (see quote)

env NODE_ENV=test mocha --reporter spec will use the declared environment variable in a natively cross platform fashion, but the key is it is used by npm in an ad hoc and one-time fashion, just for the npm script execution. (It's not set or exported for future reference.) As long as you're running your command from the npm script, there's no issue. Also, the "&&" must be removed when doing it this way.

This amounts to a very straightforward change for me:

- ELECTRON=true nuxt build --prerender && electron-builder
+ env ELECTRON=true nuxt build --prerender && electron-builder

That's a one word difference. Just prepend env to the start.

I am going to test that locally to see if it still builds the dev app appropriately when handled that way (testing the caveats) (NOTE: It seems to work), but either way I will likely push that and see what the build is like after.

thombruce commented 1 month ago

The new one to watch: https://github.com/thombruce/toodles/actions/runs/10340716726

thombruce commented 1 month ago

Windows build successful!

I don't have a lot of faith that the built application will just work. It might, but I haven't yet done a lot to tweak my specific config(s). If it does, great! If not... more work.

Next step will be publishing.

Since I'm not using Electron Forge, some of the information in the article I'm basing my build steps on is irrelevant. That article is here: https://dev.to/erikhofer/build-and-publish-a-multi-platform-electron-app-on-github-3lnd#publish

The docs for publishing with electron-builder are... confusing on this: https://www.electron.build/configuration/publish.html

Seemingly I can just add --publish onTag to my build script in package.json...? It's worth trying. We'll find out if it's worked when I... tag a release (lerna publish).

There's no way that alone works though, right?

thombruce commented 1 month ago

Linux build/publish failed. Reason: snapcraft is not installed, please: sudo snap install snapcraft --classic

MacOS build/publish succeeded... but I don't know why or where to if anywhere? I think nowhere - I think it hasn't been published.

Windows build/publish is ongoing at time of writing.

There are presently NO packages published on GitHub, despite what the (admittedly confusing) docs suggested (that if GitHub variables were detected, these would indicate that the package should be published to GitHub). Maybe I need to explicitly add those variables to the tasks in the workflow.

thombruce commented 1 month ago

lerna is forever a learning process...

What I should have done is tagged the previous as an alpha or release candidate... because now lerna publish will not publish unless I make changes to my packages.

Also, I should have tagged it as an RC because that's just good practice...

...so anyway, all packages are now at v0.1.1-alpha because I've run lerna publish --force-publish...

It's just the easiest way out of this mess. I need to document an official approach to releasing for this and other packages... and add protection to the main branch ideally. Come up with a workflow for that case. Yeah...

Anyway, I added GH_TOKEN variables to all jobs and... we'll see if that works, though I don't expect it to straight away. I expect now it could work, assuming electron-builder has the in-built capability to detect the commit tag and communicate the newly built files to the GitHub publishing mechanism...

...but I still expect to have to do more to get this properly working, and that doesn't seem to be appropriately documented. The docs really seem to suggest that this is all we really need.

thombruce commented 1 month ago

A good sign actually: The process did fail but only because (checking the Mac build) it couldn't determine the appropriate repository, and suggested I needed to add a repository value to the package.json. Makes sense; it's a monorepo!

That does suggest it is trying to publish directly to the repository releases... so that's great! That means the functionality probably is built into electron-builder and I really won't have to fiddle with this configuration more.

That said, we're getting these errors now:

Error #1 --------------------------------------------------------------------------------
HttpError: 403 Forbidden
"method: post url: [https://api.github.com/repos/thombruce/toodles/releases\n\n](https://api.github.com/repos/thombruce/toodles/releases/n/n)          Data:\n          {\"message\":\"Resource not accessible by integration\",\"documentation_url\":\"https://docs.github.com/rest/releases/releases#create-a-release\",\"status\":\"403\"}\n          "

Still means it is trying to execute the expected behaviour... but is failing due to lack of authorisation.

Suggests all we need is to add permissions to our workflow?

thombruce commented 1 month ago

Linux build seems to not like the absence of snapcraft (perhaps we should install that manually):

  • publishing      publisher=Github (owner: thombruce, project: toodles, version: 0.1.1-alpha.2)
  • uploading       file=Toodles-0.1.1-alpha.2.AppImage provider=github
  • publishing      publisher=Snap Store
  • uploading       file=app_0.1.1-alpha.2_amd64.snap provider=snapStore
  ⨯ snapcraft is not installed, please: sudo snap install snapcraft --classic  
  ⨯ /home/runner/work/toodles/toodles/node_modules/app-builder-bin/linux/x64/app-builder process failed ERR_ELECTRON_BUILDER_CANNOT_EXECUTE
Exit code:
1  failedTask=build stackTrace=Error: /home/runner/work/toodles/toodles/node_modules/app-builder-bin/linux/x64/app-builder process failed ERR_ELECTRON_BUILDER_CANNOT_EXECUTE
Exit code:
1
    at ChildProcess.<anonymous> (/home/runner/work/toodles/toodles/node_modules/builder-util/src/util.ts:252:14)
    at Object.onceWrapper (node:events:634:26)
    at ChildProcess.emit (node:events:519:28)
    at maybeClose (node:internal/child_process:1105:16)
    at Process.ChildProcess._handle.onexit (node:internal/child_process:305:5)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

...although an AppImage does appear to have been published.


Mac has successfully published


Windows... we're always waiting on Windows. Hurry up, Windows build!

thombruce commented 1 month ago

Windows also published successfully.

It's a setup package.

That's okay, and I would love to keep that around, but I would also like to be publishing a portable version of the app. Something else to look into.

thombruce commented 1 month ago

That said, I wonder... where will todo.txt be saved in the non-portable version? Because it's supposed to save in the same directory.

I guess download it and find out...

thombruce commented 1 month ago

Issue seen on NSIS app is present on portable too:

Not allowed to load local resource: file:///C:/Users/thom/AppData/Local/Temp/2kWKyZDtYwIjKvxShoylvyN6MgB/resources/app.asar/.output/public/index.html

index.html is not being prerendered....

Initial and clear error was this:

Error:  Named export 'orderBy' not found. The requested module 'file://D:/a/toodles/toodles/node_modules/lodash/lodash.js' is a CommonJS module, which may not support all module.exports as named exports.

I seem to have resolved this... but we still get 500 during the prerendering.

Running a build locally to see if I can replicate the issue and get more information, as as of now the error is no longer clear on CI.

Unfortunately... it doesn't look any clearer locally. It is still getting a 404 for index (and for /settings - less critical).


I constantly have to be reminded that I have set failOnError to false in TNT... Let's try setting that to true...

Still not clear:

ℹ Initializing prerenderer                                                                           nitro 6:08:02 PM
ℹ Prerendering 3 initial routes with crawler                                                         nitro 6:08:04 PM
  ├─ /404.html (62ms)                                                                                 nitro 6:08:04 PM
  ├─ /200.html (61ms)                                                                                 nitro 6:08:04 PM
  ├─ / (271ms)                                                                                        nitro 6:08:05 PM
  ├─ /_payload.json?d40506b9-6436-445e-856e-38492af52eb1 (13ms) (skipped)                             nitro 6:08:05 PM
  ├─ /./ (14ms)                                                                                       nitro 6:08:05 PM
  │ ├── Error: [404] Page not found: /./
  │ └── Linked from /
  ├─ /./settings (8ms)                                                                                nitro 6:08:05 PM
  │ ├── Error: [404] Page not found: /./settings
  │ └── Linked from /
  ├─ /_payload.json (2ms)                                                                             nitro 6:08:05 PM
  ├─ /settings (39ms)                                                                                 nitro 6:08:05 PM
  ├─ /settings/_payload.json?d40506b9-6436-445e-856e-38492af52eb1 (2ms) (skipped)                     nitro 6:08:05 PM
  ├─ /settings/_payload.json (2ms)                                                                    nitro 6:08:05 PM
                                                                                                      nitro 6:08:05 PM
Errors prerendering:
  ├─ /./ (14ms)                                                                                       nitro 6:08:05 PM
  │ ├── Error: [404] Page not found: /./
  │ └── Linked from /
  │ └── Linked from /settings
  ├─ /./settings (8ms)                                                                                nitro 6:08:05 PM
  │ ├── Error: [404] Page not found: /./settings
  │ └── Linked from /
  │ └── Linked from /settings
                                                                                                      nitro 6:08:05 PM

 ERROR  Exiting due to prerender errors.                                                                    6:08:05 PM

  at prerender (/home/thombruce/dev/thombruce/toodles/node_modules/nitropack/dist/chunks/prerender.mjs:220:11)
  at async /home/thombruce/dev/thombruce/toodles/node_modules/nuxt/dist/index.mjs:3615:7
  at async build (/home/thombruce/dev/thombruce/toodles/node_modules/nuxt/dist/index.mjs:5479:5)
  at async Object.run (/home/thombruce/dev/thombruce/toodles/node_modules/nuxi/dist/chunks/build.mjs:94:5)
  at async runCommand$1 (/home/thombruce/dev/thombruce/toodles/node_modules/nuxi/dist/shared/nuxi.6aad497e.mjs:1648:16)
  at async runCommand$1 (/home/thombruce/dev/thombruce/toodles/node_modules/nuxi/dist/shared/nuxi.6aad497e.mjs:1639:11)
  at async runMain$1 (/home/thombruce/dev/thombruce/toodles/node_modules/nuxi/dist/shared/nuxi.6aad497e.mjs:1777:7)

 ERROR  Exiting due to prerender errors.                                                                    6:08:05 PM

error Command failed with exit code 1.

"Page not found" is as much as we're getting.

thombruce commented 1 month ago

Looks like adding ssr: false to my Nuxt config resolves the issue. Obvious really.

This should have been in my TNT Electron config anyway... but it hasn't been necessary till now. Added and will push it up the chain.

thombruce commented 1 month ago

This is what we're greeted with when running the app:

image

[nuxt] error caught during app initialization Error: Page not found: /C:/Users/thom/AppData/Local/Temp/2kWRaneFpWpZmlLdPyjdg1drRY4/resources/app.asar/.output/public/index.html

The complaint is once again that it is unable to find index.html - despite this...

We can navigate back home from there, and from this point...

...and that's it.

There is no bar to add new todos. There are no todos shown (this is expected).

I'll try creating a dummy todo.txt in the same folder...

...but it does nothing.

So... The application is being built and the features of TNT/TNT Electron are present and working, but... the actual index of the Toodles app is not there, and so no other parts of Toodles itself are being loaded.

That's weird.

Here's a thought... Are layers the problem?

Because I do attempt to load index.html via the Electron main process, and this lives in the TNT Electron layer. Is it loading the built index.html of TNT Electron instead of Toodles?

That could very well be the issue.

There are tricks I've exercised when loading various files - mainly Tailwind:

import { fileURLToPath } from 'url'
import { dirname, join } from 'path'

const fs = require('fs')

const currentDir = dirname(fileURLToPath(import.meta.url))

export default defineNuxtConfig({
  tailwindcss: {
    configPath: fs.existsSync('./tailwind.config.js') ? './tailwind.config.js'
      : fs.existsSync('./tailwind.config.ts') ? './tailwind.config.ts'
      : join(currentDir, './tailwind.preset.js'),
    cssPath: join(currentDir, './assets/css/tnt.css'),
  },
})

Notice that we're checking for the existence of Tailwind's config in the consuming child's files before falling back on the parent layer. We perform a similar trick in Nuxt Electron to either use the user's main.ts or fallback on our own...

In TNT's main.ts for Electron, we...

import path from 'path'
const distPath = path.join(__dirname, '../.output/public')
win.loadFile(path.join(distPath, 'index.html'))

This is not executing the trick, right? Perhaps it should. Could we achieve it here?

Looking back at the error text, there's no reference to the paths of any Nuxt layers... it's just looking for a index.html at some generated endpoint. Maybe that generated endpoint is generated FOR the index.html of... the parent layer?

Maybe we'll have to move main.ts to the child project.

Tricky. At this point, I'm somewhat confident it is a layers issue though... and the trick discussed might work.

thombruce commented 1 month ago

Overcoming one part of the problem above (the initial 404) is maybe as simple as activating hash mode (again, this is something I ought to have done already on TNT Electron but elected not to in case the user wanted to set this themselves or wanted different behaviour... but it's done now). This is the solution given here: https://github.com/nuxt/nuxt/issues/15629

However, people there say that upon navigating home everything did seem to then work. That's not my experience. The app I'm seeing is empty.

So...

Maybe I'm bang on the money above where I talk about loading the wrong application (loading TNT Electron's generated index.html rather than Toodles').

What else could account for it "working" but... not having our application-specific contents?

Gonna download the latest Windows build when it's done and go hunting for errors. Will report back here.

thombruce commented 1 month ago

We still get an error, it's just not particularly useful:

[nuxt] error caught during app initialization TypeError: Cannot read properties of undefined (reading '_s')

"reading _s"? What does that mean? We don't have any variables or properties called "_s" and I think this is happening because the code is being obfuscated by the build... maybe.

So where do we "read properties" of any object?

Outside of a for-in (which should only execute when we've successfully loaded todos, our index checks properties on a newTodo... but newTodo is itself a ref and it's not ever undefined.

We do also fetchTodos() in our index. The first thing we do here is useTntApi().loadFile('todo.txt') and our linter does complain about the presence of useTntApi() - "cannot find name 'useTntApi'.". It's supposed to be automatically imported, though I have previously written a sort of pseudo-import for it in this file... as well as attempted to import it straight from the source. I'll try that again now...

I'm not sure I had seen those warnings previously, you know? Those ones I said weren't relevant. Here's a sample:

 WARN  [plugin:vite:reporter] [plugin vite:reporter]                                                        8:43:24 PM
(!) /home/thombruce/dev/thombruce/tnt/packages/core/components/global/ToastStack.vue is dynamically imported by /home/thombruce/dev/thombruce/tnt/packages/core/components/global/ToastStack.vue?nuxt_component=async&nuxt_component_name=ToastStack&nuxt_component_export=default but also statically imported by /home/thombruce/dev/thombruce/tnt/packages/core/app.vue, dynamic import will not move module into another chunk.

We get that same warning about several of my custom TNT components now. If I'm right that I actually haven't seen these before... that could indicate we've fixed the issue. That is to say the warnings aren't relevant, but their presence indicates the issue we're looking at may be gone.

But I've also not been looking for them previously. Only a test of the built application will tell if we've actually made any progress... and I have to wait about ten minutes for the Windows build (why is it so slow!!!).

thombruce commented 1 month ago
Error:  Nuxt Build Error: [vite]: Rollup failed to resolve import "@thombruce/tnt/composables/tntApi" from "/home/runner/work/toodles/toodles/packages/app/stores/todos.ts".

But what if I explicitly list @thombruce/tnt in my package.json? Let's see...

thombruce commented 1 month ago

I can't test this locally (it's succeeding locally), so I need to push. I don't always need to tag my releases though, so I haven't...

The builds are attempted anyway, they're just only published if I tag the commit.

thombruce commented 1 month ago

Build now succeeds on Linux... waiting on Mac and Windows, but given they had the same error, I think we can press on.

With that succeeding, I will tag the commit and move onto download and local testing once the next Windows release is ready.

thombruce commented 1 month ago

Important Aside

If this does work, what does it actually tell me? What should I do?

Because in order to achieve this I have added @thombruce/tnt to my dependencies and directly imported the exported function from the composable directory there. In order to not require this, I could...

Alias useTntApi() in a composables directory in @thombruce/tnt-electron... but with how auto-imports work, I fear this would just be overwriting the base composable.

I could move it to TNT Electron... but I think we want it on Core. We want it on Core because it is going to be responsible for determining whether the app is built for Electron... or mobile... or web... and all of those are going to be installed in different places, different ways...

We could alias it in TNT Electron, just not in composables. We could create an exports file or directory for this...

And we could... move it to yet another layer, "tnt-api", making the assumption that the API functionality isn't always going to be wanted.

For now! I'm gonna press ahead without answering that. But yeah, this does give us an extra wrinkle to think about.

thombruce commented 1 month ago

Really, why does Windows take so long? I wrote all of that (^^^) while waiting for it to publish! It's still going...

thombruce commented 1 month ago

Excellent news!!!

That was the bloody problem! It now... doesn't work.

But that's okay. The new error is an obvious one:

Error: Error invoking remote method 'load-config': Error: ENOENT: no such file or directory, open 'C:\Users\thom\AppData\Local\Temp\2kWkDuyRvCNa56RZfwKaA6FHCfp\tnt.config.json'

It's trying to load a config file from AppData. Really it should be succeeding and pressing on without it if the file doesn't exist. It should create it if it doesn't.

Uhmmm... even if we could get past this, because the todo.txt file is handled pretty much identically, the problem is going to occur then too.

I was under the impression that fs.readFile() would... work like that. And fs.writeFile() would create the file if it didn't exist.

Maybe it's complaining that the directory doesn't exist? With portable apps, this is a sort of virtual directory that's created and deleted as and when needed, right? See: AppData\Local\Temp - it's Temp.

So in any case... that's still the wrong location. We want our portable app to work with files in its directory. Maybe if we solve that, the issue itself resolves?

The solution (if this works: https://stackoverflow.com/a/46647041) is not ideal. The env variable in that answer is apparently electron-builder specific so... we'd lose some agnosticism. E.g. If we wanted to use Forge instead or if we wanted to migrate to Tauri (though this is in an Electron-specific file, so no worries there). It also breaks portable/non-portable agnosticism... maybe. I don't think it does right now, but keep that sort of thing in mind.

thombruce commented 1 month ago

I now have portable app working with files in the same directory. Great! But that issue lingers.

The advice I've seen with regards to checking a file's existence is... just try fs.readFile(). This seems to be bad advice, right? The app is throwing an error and not loading.

Let's take a look...

    return new Promise((resolve, reject) => {
      fs.readFile(join(String(process.env.PORTABLE_EXECUTABLE_DIR), "tnt.config.json"), "utf8", function (error, file) {
        if (error) {
          reject(error)
          return
        }
        resolve(JSON.parse(file))
      })
    })

And here's where it's invoked:

      if (isElectron()) {
        return await window.api.invoke('load-config')
      }

So we await, and because the file doesn't exist we wind up with a reject... which essentially means we don't end up with what we expect.

This used to work, but I rewrote it to use promises and I guess I didn't test the case of a file not existing, so I didn't know how that was going to behave and... now I do.

If I instead do not reject(error) but resolve({ colorMode: "dark" })... we get the expected behaviour.

So not an fs.readFile issue. Just bad use of a promise on my part.

We'll leave that sort of logic in place for writeFile though:

        fs.writeFile('tnt.config.json', JSON.stringify(newData, null, 2), (error) => {
          if (error) {
            reject(error)
            return
          }
          resolve(newData)
        })

...though, by the way, I've missed a PORTABLE_EXECUTABLE_DIR usage there and need to amend that.

We'll leave the reject in place because if we fail to write to the file... it doesn't make sense to return the data - the data that failed to write to the file? No, we want the error in that case.

And the loading/writing of arbitrary files should... mirror my changes. Yup. They want to behave exactly the same way for files we'll be storing application data in. Nice.

thombruce commented 1 month ago

Note that we do still need to return after the resolve in order to not also invoke the second resolve which tries to parse non-existent JSON:

    return new Promise((resolve, reject) => {
      fs.readFile(join(String(process.env.PORTABLE_EXECUTABLE_DIR), "tnt.config.json"), "utf8", function (error, file) {
        if (error) {
          resolve({})
          return
        }
        resolve(JSON.parse(file))
      })
    })
thombruce commented 1 month ago

So confident was I in that fix, apparently, I've went ahead and lerna published v0.1.1.

I think that's safe. I know we're at least operational on Windows and the application is loading and behaving just as it should.

I do know that, provided the files actually did exist, I was able to load the config and todos and write to the files too. Now that I've addressed the scenario where they do not previously exist... we should be fine. Famous last words? Me? Never. I'm not famous.