node-convict icon indicating copy to clipboard operation
node-convict copied to clipboard

After 6.0.0 ?

Open A-312 opened this issue 5 years ago • 7 comments

@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:
    [email protected] x 40,531 ops/sec ±1.59% (86 runs sampled)
    my-fork@latest x 134,203 ops/sec ±1.52% (90 runs sampled)
    [email protected] 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 avatar May 03 '20 05:05 A-312

@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!

madarche avatar May 03 '20 10:05 madarche

@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! 😃

madarche avatar May 09 '20 15:05 madarche

  • 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 🤔~~ )

A-312 avatar May 10 '20 12:05 A-312

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?

madarche avatar May 10 '20 14:05 madarche

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

madarche avatar May 10 '20 16:05 madarche

Let finish #353 ?

A-312 avatar May 11 '20 12:05 A-312

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

madarche avatar May 20 '20 06:05 madarche