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 needed for multiple config file formats! #33

Open
MercerK opened this issue Jul 26, 2022 · 16 comments
Open

Support needed for multiple config file formats! #33

MercerK opened this issue Jul 26, 2022 · 16 comments

Comments

@MercerK
Copy link
Contributor

MercerK commented Jul 26, 2022

I think this broke a few versions ago. Right now, it defaults to grakkit/index.js every time. You use to be able to use a config to change the path.

@MercerK
Copy link
Contributor Author

MercerK commented Jul 26, 2022

The alternative seems to be package.json and leveraging the main field instead.

@Yu-Vitaqua-fer-Chronos
Copy link
Member

Maybe we should make the package.json the standard instead of config.yml? (Just an idea of course, would appreciate other opinions)

@ghost
Copy link

ghost commented Jul 30, 2022

config.yml has not been the standard since Grakkit v5. package.json is the standard and will be going forward!

@ghost ghost closed this as completed Jul 30, 2022
@MercerK
Copy link
Contributor Author

MercerK commented Jul 31, 2022

config.yml has not been the standard since Grakkit v5. package.json is the standard and will be going forward!

I don't agree with this.

  • That violates our second principle, as we're making this suddenly opinionated.
  • package.json is not a graal-specific item, it is a product produced by NodeJS.
  • Grakkit doesn't support NodeJS at the moment, so requiring a package.json becomes immediately misleading.

What happens when the user wants to use Deno, which is a NodeJS alternative?

What about when package.json makes significant schema changes?

Or what happens when the user doesn't want to use package.json for plugin configurations, to minimize the amount of noise?

How about when we need to extend additional customizations onto our config?

Or the user has a need to have main property within package.json point to something else? Do they now need multiple package.json? Users could run into issues with workspaces due to this.

A good portion of our users right don't use a single method. Everyone does something different.

I propose that instead of making package.json the standard, we give the users the option.

Grakkit-specific configuration can be driven by .grakkitrc, package.json, modules.json, or anything else.

@ghost
Copy link

ghost commented Jul 31, 2022

Hmm, good point about deno... and now that you mention it, it does break the principle. I suppose I never gave it much thought since I didn't see a better alternative to choosing something like package.json

@ghost ghost reopened this Jul 31, 2022
@Yu-Vitaqua-fer-Chronos
Copy link
Member

config.yml is the standard really, so that should be used for the Spigot platform, for Minestom it's likely a bit different so whatever works there

@ghost
Copy link

ghost commented Jul 31, 2022

@Mythical-Forest-Collective I would normally agree but since Grakkit's common lib is, well, common, it makes sense to have a unified config format across all platforms. If not for that, it'd make it more complicated for people, like if two devs want to share their setup of files with another and they use different platforms, they'd have to change files around.

@Yu-Vitaqua-fer-Chronos
Copy link
Member

Hm that's fair, though we will need a way to allow people to define where the file gets saved since every platform is different

@MercerK
Copy link
Contributor Author

MercerK commented Jul 31, 2022

I think we can check for a set of supported files?

  • Does grakkit.json or .grakkitrc file exist? Load that JSON.
  • Does config.yml exist? Load that YML.
  • Does package.json exist? Load that JSON.

@ghost
Copy link

ghost commented Jul 31, 2022

@MercerK I can see that being nice for the backwards-compatibility, though it would add a lot of extra code to the common library for checking an ever-growing list of supported configs. And how would we decide which ones take priority?

@MercerK
Copy link
Contributor Author

MercerK commented Jul 31, 2022

As an example, NodeJS Plugins do support configs coming from different files; it just depends on what we support.

For example, eslint has .eslintrc, eslintrc.json, eslintrc.js, or as part of the package.json. Jest has something similar.

We can just set a read order and whichever file we find first is the one we use.

However, it doesn't matter what file they use, the configuration schema should be the same.

@ghost
Copy link

ghost commented Jul 31, 2022

Hmm... wouldn't Grakkit work differently to ESLint due to platform-specific options in the config?

@MercerK
Copy link
Contributor Author

MercerK commented Jul 31, 2022

Why would grakkit's configuration behave differently from the eslint configuration? It should behave the same across all files in all the same ways. It should not matter what platform the user is leveraging, that is more of a schema problem than a file problem.

@ghost
Copy link

ghost commented Jul 31, 2022

In the future, there might be config options for platform-specific things built into Grakkit's java code. Take bukkit command registry as an example. I could see Grakkit using config.yml to toggle that functionality. Such an option may not be present in something like fabric or minestom. Wouldn't that be confusing? How would people know what works where?

@MercerK
Copy link
Contributor Author

MercerK commented Jul 31, 2022

No matter the platform, you can solve that with a single configuration schema with documentation (which is key). You don't necessarily need to abstract into multiple files for specific platforms.

We don't have platform-specific configurations now, so I don't think scoping a future "problem" as a blocker to this is ideal.

@ghost
Copy link

ghost commented Jul 31, 2022

Hm, that's true. Though, if we do end up adding platform-specific configs, I think we can set the schema up in a way where it makes room for that. As an example...

{
   "grakkit": {
      "main": "index.js"
   },
   "bukkit": {
      "commandRegistry": true
   },
   "minestom": {
      "commandRegistry": true
   }
}

For now, we'd just have "grakkit" and the "main" field, but we can extend this config if needed.

@ghost ghost changed the title config.yml no longer works. Support needed for multiple config file formats! Jul 31, 2022
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

No branches or pull requests

2 participants