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

After 6.0.0 ? #370

Open
A-312 opened this issue May 3, 2020 · 7 comments
Open

After 6.0.0 ? #370

A-312 opened this issue May 3, 2020 · 7 comments

Comments

@A-312
Copy link
Contributor

A-312 commented May 3, 2020

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

@madarche
Copy link
Collaborator

madarche commented May 3, 2020

@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
Copy link
Collaborator

madarche commented May 9, 2020

@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
Copy link
Contributor Author

A-312 commented May 10, 2020

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

@madarche
Copy link
Collaborator

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
Copy link
Collaborator

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

@A-312
Copy link
Contributor Author

A-312 commented May 11, 2020

Let finish #353 ?

@madarche
Copy link
Collaborator

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

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