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

Config file #36

Closed
wants to merge 7 commits into from
Closed

Config file #36

wants to merge 7 commits into from

Conversation

SleepyLeslie
Copy link
Collaborator

This PR implements CLI argument parsing and a configuration file for Grist Desktop. Partly addresses #25 and closes #27.
Also includes changes to rename grist-electron to Grist Desktop.

Add CLI argument parsing
Rename grist-electron to grist-desktop
@SleepyLeslie SleepyLeslie added the enhancement New feature or request label Jun 4, 2024
@SleepyLeslie SleepyLeslie self-assigned this Jun 4, 2024
Copy link
Member

@paulfitz paulfitz left a comment

Choose a reason for hiding this comment

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

Looks to be getting there! Some comments + qs. Will give it a try locally.

ext/app/electron/main.ts Outdated Show resolved Hide resolved
ext/app/electron/config.ts Outdated Show resolved Hide resolved
ext/app/electron/config.ts Outdated Show resolved Hide resolved
ext/app/electron/config.ts Outdated Show resolved Hide resolved
ext/app/electron/config.ts Outdated Show resolved Hide resolved
ext/app/electron/logins.ts Outdated Show resolved Hide resolved
ext/app/electron/logins.ts Show resolved Hide resolved
ext/app/electron/main.ts Outdated Show resolved Hide resolved
ext/app/electron/main.ts Outdated Show resolved Hide resolved
ext/app/electron/main.ts Outdated Show resolved Hide resolved
@SleepyLeslie SleepyLeslie force-pushed the sleepyleslie/config-file branch 2 times, most recently from bc42f77 to e202313 Compare June 10, 2024 18:13
Copy link
Member

@paulfitz paulfitz left a comment

Choose a reason for hiding this comment

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

Sorry for one comment review :) getting interrupted a lot

ext/app/electron/config.ts Outdated Show resolved Hide resolved
"electron": "30.0.6",
"electron-builder": "23.6.0",
"@electron/notarize": "2.3.2"
"eslint": "9.x",
Copy link
Member

Choose a reason for hiding this comment

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

How do I use eslint on this repo? npx eslint at top level didn't do anything. npx eslint within the ext directory did stuff, giving a lot of errors. Just checking what expectation is being set here?

Copy link
Collaborator Author

@SleepyLeslie SleepyLeslie Jun 12, 2024

Choose a reason for hiding this comment

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

The errors are expected (for now). There is too much legacy code that does not conform to eslint's default rules. Linting was introduced in this PR to help me check for (and auto fix) unsorted imports and missing semicolons. More linting-related changes will be added gradually.

ext/app/electron/config.ts Show resolved Hide resolved
path.join(APPDATA_DIR, "home.sqlite3")
);

const homeDBLocation = path.parse((process.env.TYPEORM_DATABASE as string)).dir;
Copy link
Member

Choose a reason for hiding this comment

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

Where did we land on trying to preserve/copy the existing home db? From my perspective as a know-nothing user, I just upgraded my version of Grist and "lost all my files".

I see that even if I find and copy across my old home db, I have still "lost all my files" because I'm a new and different user.

If I learn about the config file and set login.email to [email protected] I have still "lost all my files" because my login as the new and different user is remembered.

If I log out and log back in again, I can finally see my files.

A little bit worried about a wave of support requests as people upgrade, none of the above steps would be very obvious.

We could grit our teeth and just go through with it, is that your thinking? I know you have a modal planned.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is planned for the next PR which will implement the modal. I am thinking about a migration mechanism that detects version upgrades, performs migrations, and lets users know about the breaking changes as well as the automatic migration outcome.

Not sure if I should squeeze everything in here. I want to stop this PR from getting even larger.

Copy link
Member

@dsagal dsagal left a comment

Choose a reason for hiding this comment

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

Just a thought (sorry I only now noticed this diff): we use elsewhere in Grist an awesome little tool ts-interface-checker + ts-interface-builder (created here!) which allow one to define a typescript interface, e.g.

{
  server: { 
    port: number;
  };
  sandbox: {
    flavor?: 'pyodide' | 'gvisor' | 'unsandboxed';
    ...
  };
}

(with comments and everything), and then turn it into a "-ti.ts" file that works as a runtime descriptor. Then you could take a yaml file, and run it through ts-interface-checker that would use this descriptor, to check that the object satisfies this interface (and report reasonably informative errors).

Just sharing since it seems a nice way to do configs (vs INI files).

@SleepyLeslie
Copy link
Collaborator Author

The config change has been deferred after some discussion around implementing a config mechanism shared with core and using json/yaml instead of ini.
Will later make another PR with changes here minus the config file. Closing this for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Portable grist on Windows and simple configuration.
4 participants