mozilla/node-convict

After 6.0.0 ?

A-312 opened this issue · 7 comments

A-312 commented

@madarche What do you plan after 6.0.0 ? If I can, I will PR all my fork changes on convict because I don't plan to keep my fork active, if my fork has the only reason because I could not wait the change from mocha to jest and the merge of all my PRs.

In my fork, I did some change (without break-changes) that I can apply here:

  • Allow schema on the root (next goal: let convict parses array values like convict properties, to improve array properties validation & parsing). By "Allow schema on the root ", I mean something like that:
    {
       "default": {},
       "format": "custom-object-format"
    }
    
  • code from custom eslint to eslint-config-standard;
  • I split convict in multifiles + jsdoc:
    • In my fork we can load convict without default getters/formats with require('convict/lib/core.js') ;
  • I did some optimization on validation function in my fork to get node-convict faster:
    convict@5.2.0 x 40,531 ops/sec ±1.59% (86 runs sampled)
    my-fork@latest x 134,203 ops/sec ±1.52% (90 runs sampled)
    my-fork@6.0.2 x 37,208 ops/sec ±1.18% (92 runs sampled)
    Fastest is my-fork@latest
    
  • coverage 99%.

Do you see change/feature that can't PR on convict because don't corresponding to convict ? (each points will be a different PR)

@A-312 I really want to use as much of the good work you've done as possible. I'll come back later and answer all your questions. Thank you!

@A-312, first thanks for your patience and all the new tickets you are creating to help. And please remember that I'm not super-fast for reviewing and integrating things 😊

Then, yes re-increasing the coverage would be very appreciated. I'm sorry to have dropped the level. I could work on it, but if you can, the better and thanks again.

About eslint-related things, I really like to control and I'm much interested in the quality and the constraints on the code, so I'm not much interested in eslint-config-standard doing this job instead of us.

But about any other topic (allow schema at the root — which would be very interesting — increase speed, etc.), I wish we first improve code maintainability and code ease of understanding. I believe that could be done by:

  • making a class out of convict
  • using classes for errors (I know, as you did in your fork)
  • doing some file splitting (not too much, just what's needed and legitimate)
  • using some newly available JavaScript builtins to simplify/harden the code

Using a class instead of a function would be another breaking change and another major version of course, but simplifying convict's code is the most needed thing to do. This is what I plan to do in the future. But if you are interested in doing it faster than me, I would gladly welcome your help! 😃

A-312 commented
  • doing some file splitting (not too much, just what's needed and legitimate)

I split convict like that : https://github.com/A-312/node-blueconfig/tree/master/packages/blueconfig/lib (There is not breakchange with #353, should I call this one convict 6 or convict 7 ? Because the current version doesn't have all the change of #353 🤔 )

This splitting is very inspiring and well thought. I'd like a few renamings along those lines though:

├── main.js
├── convict.js
├── errors.js
├── config_node.js
├── schema_node.js
├── format
│   └── standard.js
└── implementor
    ├── apply.js
    ├── getter.js
    ├── parser.js
    ├── ruler.js
    └── util
        ├── parsingschema.js
        ├── utils.js
        ├── validator.js
        └── walk.js 

Rationale:

  • Use _ to split names: SchemaNode schema_node.js, etc.
  • Let's identify clearly that the Convict class comes from the convict.js file (ex-core.js)
  • Both the files convict.js (ex-core.js), config_node.js and schema_node.js are models, so let's put them in the same directory.

But everything done with class and no prototype mess. And errors should be named following classical error naming, cf. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error: ConfigError, SchemaError, etc.

Are you OK with that?

New branch created so PR can be made to it: https://github.com/mozilla/node-convict/tree/convict7

A-312 commented

Let finish #353 ?

@A-312 I'll be working on #353 in the next days.