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

Feature Request: Allow schema to be free-form #80

Open
d42ohpaz opened this issue Jun 25, 2019 · 9 comments
Open

Feature Request: Allow schema to be free-form #80

d42ohpaz opened this issue Jun 25, 2019 · 9 comments

Comments

@d42ohpaz
Copy link

Currently the implementation expects that the schema provided is the properties of an object type. For most cases, this is probably fine, but I am finding it a little limiting because I am unable to define top-level properties, such as required, propertyNames, additionalProperties, et al.

As such, I feel like projects could benefit from being able to define their own schema at any level, or default to the preexisting behavior: if the top level of the given schema doesn't define either definitions or type, then assume it's the original schema style and wrap it in its own properties key. Otherwise, allow the schema writer to define any level of the JSON schema.

I'd be willing to work on this, but only if there is buy in.

@sindresorhus
Copy link
Owner

Makes sense. I didn't think about that use-case. I just wanted to make it simple to define a schema without having to deal with the meta schema properties.

if the top level of the given schema doesn't define either definitions or type, then assume it's the original schema style and wrap it in its own properties key.

This sounds too fragile. It's very likely someone would have a type key for a different purpose.


How do you suggest we add support for this? Either a breaking change or add a separate key for top-level meta properties.

@d42ohpaz
Copy link
Author

d42ohpaz commented Sep 9, 2019

How do you suggest we add support for this? Either a breaking change or add a separate key for top-level meta properties.

I agree with your comment about guessing the schema form based on existence of a key. How about an alternative behavior:

By default treat all schemas as they currently are treated (i.e., properties of an existing object). Allow users to pass a flag with their options to opt-in their schema as free-form. No guesswork and backward compatible.

If you still don't like that, I think adding top-level keys for the meta properties would be just as good as anything else.

@sindresorhus
Copy link
Owner

Allow users to pass a flag with their options to opt-in their schema as free-form.

Can you show an example of how that would look like?

@d42ohpaz
Copy link
Author

d42ohpaz commented Sep 9, 2019

I was imaging it to look like:

const Conf = require('conf');

const config = new Conf({implicitObjectRoot: false}); // defaults to true for BC

config.set({
    type: 'object',
    additionalProperties: {type: "string"},
    properties: {unicorn: '🦄'},
});
console.log(config.get('unicorn'));
//=> '🦄'

config.set('number', 1234); // causes validation error
config.set('name', 'pinky'); // is allowed due to additionalProperties definition

@sindresorhus
Copy link
Owner

You meant this right?

const Conf = require('conf');

const config = new Conf({
	implicitObjectRoot: false,
	schema: {
		type: 'object',
		additionalProperties: {type: "string"},
		properties: {unicorn: '🦄'},
	}
}); // defaults to true for BC

console.log(config.get('unicorn'));
//=> '🦄'

config.set('number', 1234); // causes validation error
config.set('name', 'pinky'); // is allowed due to additionalProperties definition

If so, I would be ok with that. However, thinking more about it, giving two roles to schema would complicate the TypeScript typings. I think it would be easier to just have a separate property like this:

const Conf = require('conf');

const config = new Conf({
	rootSchema: {
		type: 'object',
		additionalProperties: {type: "string"},
		properties: {unicorn: '🦄'},
	}
}); // defaults to true for BC

console.log(config.get('unicorn'));
//=> '🦄'

config.set('number', 1234); // causes validation error
config.set('name', 'pinky'); // is allowed due to additionalProperties definition

Suggestions for better name welcome.

@d42ohpaz
Copy link
Author

d42ohpaz commented Sep 9, 2019

Yes, you're right I meant to pass the schema in with the config. Good catch.

I would agree with you about using rootSchema instead of schema, except that JSON can be anything from null to an object. This would mean Conf would need to then take on the burden of type detection and using the correct JSON Schema format for what was passed in. If that format ever changed, then it could break everybody's builds until the project caught up.

Personally I don't see a problem forcing the users who opt-in to this behavior to manage the entirety of the schema themselves. As for Typescript typings, if I'm understanding the code correctly you depend on json-schema-typed for that, and they would support valid JSON Schema syntax. Or are you referring to something else in this case?

@sindresorhus
Copy link
Owner

except that JSON can be anything from null to an object.

Not exactly sure what you mean by that. Conf would only every support an object at the top-level, so if the user tries to set something else, we would throw a useful error.

As for Typescript typings, if I'm understanding the code correctly you depend on json-schema-typed for that, and they would support valid JSON Schema syntax. Or are you referring to something else in this case?

I just meant that having multiple types for one options complicates the types. Having two options, each with only one type each makes it much simpler.

@strophy
Copy link

strophy commented Dec 11, 2020

Would implementation of this feature also allow definitions at the top level? I'm trying to migrate a custom config solution to use conf instead, but I can't seem to successfully apply definitions from our existing schema at any level - I have tried wrapping the top level in a dummy property but it doesn't work. Is there any way we can use definitions with the current behaviour?

@sindresorhus
Copy link
Owner

@strophy Yes, it would support anything ajv supports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants