adonisjs / rfcs

💬 Sharing big changes with community to add them to the AdonisJs eco-system
52 stars 6 forks source link

Proposal to remove `start/app.js` in favor of `.adonisrc.json` file #21

Closed thetutlage closed 4 years ago

thetutlage commented 4 years ago

In v5 I have created a new file called .adonisrc.json. The purpose of this file is to serve the runtime and the build tooling (for typescript) and also the ace commands.

Example of .adonisrc.json

{
  "directories": {
    "httpControllers": "App/Controllers/Http"
  },
  "preloads": [
    {
      "file": "start/routes"
    },
    {
      "file": "start/kernel"
    }
  ],
  "metaFiles": [
    ".env",
    ".adonisrc.json",
    ".gitignore"
  ]
}

By reading the contents of the above file, AdonisJs can perform actions of following nature.

  1. The build command of AdonisJs will look up the static files to copy to the build directory.
  2. The runtime of AdonisJs can use preloads array to preload the files before running the server.
  3. The make commands of AdonisJs can look for directories object to find the path for creating a new controller.

So basically, this file contains the workspace and the runtime configuration for AdonisJs applications.

Rewind

In the current version of AdonisJs, we have a file called start/app.js. If you look at this file closely, you will releasize that it is basically a configuration file that exports.

You can have behavior in this file as well (since it is a Javascript file), however, you will be fairly limited in what you can do inside this file.

For example: You cannot import and use providers in this file, since providers are not registered and booted yet.

In brief, even though start/app.js is a Javascript file, it's main purpose is to export some static values.

Back to the Present

Now, if we continue with .adonisrc.json and start/app file. AdonisJs will basically end up having 2 configuration files.

I propose to remove start/app file in favor of .adonisrc.json file and it has handful of upsides and downsides.

Upsides

  1. Since it is a JSON file, it can be parsed by other programming languages as well.
  2. All configuration will be in place.
  3. AdonisJs can update this file automatically after you install a dependency. For example: After installing @adonisjs/lucid, we can safely update this and add @adonisjs/lucid providers to the providers array. Doing same with a Javascript file is bit tricky, coz of it's dynamic behavior.
  4. Ace commands can load this file to make various decisions, without loading the entire app.

Downsides

  1. Since it is a JSON file, we cannot have comments inside it.
  2. The file cannot contain dynamic behavior. For example: Writing an if statements to enable/disable providers based upon the current NODE_ENV value.
  3. It can get verbose, since we may have to expose proper API to handle commonly required conditionals.

What happens if this change is approved?

  1. All of the workspace configuration will be moved to .adonisrc.json file. It means, that you will have to defined providers, aceProviders and all other values inside .adonisrc.json and not start/app file.
  2. The start/app file will be removed.
targos commented 4 years ago

FTR, I currently need to write JS for two reasons in start/app.js:

const commands = fs
  .readdirSync(path.join(__dirname, '../app/Commands'))
  .map((command) => `App/Commands/${command}`);

It would be nice if the make:command command updated .adonisrc.json automatically.

Generally, I think this proposal is good, because JSON can be more easily updated/manipulated by install scripts or analyzed by tooling.

thetutlage commented 4 years ago

@targos The providers will be resolved relative from the project root. So you can write.

{
  "providers": [
    "./providers/AppProvider"
  ]
}

Regarding dynamic commands. I already realized this issue, so have added support in ace, where a command can export an array of subpaths for actual commands. This is how it looks like

.adonisrc.json

{
  "commands": [
    "./app/Commands"
  ]
}

inside ./app/Commands/index.js, you can do

module.exports = fs.readdirSync(path.join(__dirname, '../app/Commands'))

This approach will also keep your .adonisrc.json file tidy. For example: The official set of AdonisJs commands can be added with just one entry

{
  "commands": [
    "@adonisjs/commands"
  ]
}

and the entry point of that module exports an array of actual paths for the commands

betaWeb commented 4 years ago

Hi there,

Why not to keep both approches : app/start.js and .adonisrc.json, and specify in the documentation that, for example, .adonisrc.json prevails over app/start.js. Or, event better, bring together the best of both worlds and propose a root configuration file adonis.config.js :)

thetutlage commented 4 years ago

@betaWeb

Won't mind having .adonisrc.json or .adonisrc.js. Similar to what Webpack and babel does. (They call it cosmiconfig)

But wanted to set the expectations right, that this file is meant to be a config file and the application is not booted when this file is loaded. So, do not right logic inside it.

annymosse commented 4 years ago

I think for current time @betaWeb have right and it's a good decision until the future development provide more features and the full path ,even you need the best practices of both ,then both of them have pros & cons ,I believe it's good idea to use the positive of both later when you get the full puzzle you can make a final decision better than switching between the new plan and the old plan to make the framework stable and handle the future unknown issues, still only my idea :p

MZanggl commented 4 years ago

I really like how this completely eliminates the need for something like autodiscovery and even better, doesn't hide which providers are registered. I you want to go with the cosmiconfig approach and since the feature of "adding the providers automatically" would only work for the .adonisjsrc.json file, would that feature be scrapped entirely or just limited to the json file?

thetutlage commented 4 years ago

I like that you mentioned autodiscovery as well. Yes, having a proper top level configuration file does help in automatically registering providers without relying on implicit features like autodiscovery.

We can have this feature for both .json and .js files. Since, it's not that hard to convert the JS file to AST, mutate the AST and write it back.

Now someone may ask, if updating the .js file is that easy, then why remove start/app.js file. The answer to that is, the intent of start/app.js is blurry.

After this proposed change, we are saying loud, that .adonisrc(.json|.js) is a pure config file, meant to be read and updated by the tooling of AdonisJs, so please do not complicate this file.

The cosmiconfig is broader concept than just having a .json or .js file. So I am not 100% sold to cosmiconfig, but to answer your question, we can still auto update the .adonisrc regardless of the extension.

MZanggl commented 4 years ago

Very nice. In this case support for .adonisjsrc.js is something that could always be added later once valid use cases are found.

betaWeb commented 4 years ago

@thetutlage,

It can be pure configuration logic. A basic example could be define a config variable differently, relative to the environment (dev, test, prod etc), no matter if the application booted or not. In this case, a .js file seems more pertinent than a .json file ; it let more freedom to user to define his configuration in a very precise and custom way :)

And keep the app/start.js file let an entrypoint to custom all that appeal to the Adonis application.

RomainLanz commented 4 years ago

A basic example could be define a config variable differently, relative to the environment (dev, test, prod etc)

If you have different values depending on the environment you should use the environment variables/file. It's exactly its purpose.

thetutlage commented 4 years ago

All of the feedback has been very positive. So I will consider the proposal as approved.

Thanks everyone 😄