After 6.0.0 ?
A-312 opened this issue · 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')
;
- In my fork we can load convict without default getters/formats with
- 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! 😃
- 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 theconvict.js
file (ex-core.js
) - Both the files
convict.js
(ex-core.js
),config_node.js
andschema_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