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

Support for multiple configuration file formats(electron-builder) #819

Closed
mato533 opened this issue Nov 11, 2024 · 5 comments · Fixed by #823
Closed

Support for multiple configuration file formats(electron-builder) #819

mato533 opened this issue Nov 11, 2024 · 5 comments · Fixed by #823
Labels
electron-builder enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Milestone

Comments

@mato533
Copy link
Contributor

mato533 commented Nov 11, 2024

Description

We are using electron-builder with a YAML formatted configuration file.
When I tried to run E2E tests using this wonderful library, I realized that it only supports configuration files in json format.
Referring to the official documentation, electron-builder supports not only Json but also YAML and TOML format files.

I think it should also support automatic loading using configuration files other than Json.

Related code

if (builderDependencyDetected && !builderConfig) {
// if builder config is not found in the package.json, we attempt to read `electron-builder.json`
const builderConfigFileName = 'electron-builder.json';
const builderConfigPath = path.join(rootDir, builderConfigFileName);
try {
log.info(`Reading Builder config file: ${builderConfigPath}...`);
const data = await fs.readFile(builderConfigPath, 'utf-8');
builderConfig = JSON.parse(data);
} catch (_e) {
log.warn('Builder config file not found or invalid.');
}
}

Workaround

Specify the appBinaryPath option in my capabilities.

@goosewobbler goosewobbler added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed electron-builder labels Nov 12, 2024
@goosewobbler goosewobbler added this to the 7.x.y milestone Nov 12, 2024
@goosewobbler
Copy link
Member

Agreed, this should be a thing.
YAML support should be pretty easy by using https://www.npmjs.com/package/yaml.
TOML equivalent only supports 0.4.0 of the spec: https://www.npmjs.com/package/toml
However there is smol-toml which looks like a good modern option for parsing TOML.

@mato533
Copy link
Contributor Author

mato533 commented Nov 12, 2024

@goosewobbler
Thank you for considering.
I think there are 2 options resolving this issue.
Do you think that policy Option.1 is desirable?(Sorry if your comment took Option.2 into account.)

  1. Parses the configuration file independently
    Implemented using libraries suggested in the comment.

  2. Reuse electron-builder's configuration file reading implementation
    Use app-builder-lib. This library is used internally by electron-builder when reading configuration files.
    However, since it is used internally, I think it may be subject to change.
    (In this case, findAndReadConfig may be useful.)

@mato533
Copy link
Contributor Author

mato533 commented Nov 12, 2024

By the way, when I implemented Option.2 experimentally, it seems to work, but a mysterious error occurs in the unit test of wdio-electron-service. I don't know the root cause due to my lack of knowledge, but it seems that the mock using nock is causing the problem...

@goosewobbler
Copy link
Member

goosewobbler commented Nov 12, 2024

@mato533 ah yes using app-builder-lib will be fine - This will only be used for electron-builder so no need to roll our own if we can use their config parsing code.

If you want to submit a PR I'll happily review and look at fixing the units. We should probably also add some new config fixtures for the electron-builder YAML and TOML cases.

@mato533
Copy link
Contributor Author

mato533 commented Nov 13, 2024

@goosewobbler
Thank you for the reply.
I have created PR(#823) using app-builder-lib and commented about the issue.
I apologize for my incompetence, but I would appreciate your help in reviewing the code and resolving the issue.

By the way, when I implemented Option.2 experimentally, it seems to work, but a mysterious error occurs in the unit test of wdio-electron-service. I don't know the root cause due to my lack of knowledge, but it seems that the mock using nock is causing the problem...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
electron-builder enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants