Skip to content

Conversation

yavorona
Copy link
Contributor

Summary

Convert project_config_schema and json_schema_validator modules from JS to TS.

Test plan

  • Manual testing using local build and linked Node app
  • Existing unit tests

@yavorona yavorona requested a review from a team as a code owner July 17, 2020 04:38
@yavorona yavorona assigned yavorona and unassigned yavorona Jul 17, 2020
@yavorona yavorona changed the title Convert project_config_schema and json_schema_validator modules to TS feat: Convert project_config_schema and json_schema_validator modules to TS Jul 17, 2020
* @return {boolean} true if the given object is valid
*/
export var validate = function(jsonObject) {
export function validate(jsonObject: Record<string, unknown>): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think instead of using Record... you wanna just use any so we can validate anything against this. An alternative type to use maybe would be JSON

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried any before , which gives Argument 'jsonObject' should be typed with a non-any type error. Did not think of JSON, thanks @mikeng13!

export default schema;
const schema = schemaDefinition as JSONSchema4

export { schema }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to keep the export default schema here since we will only have main export from this module.

Then down below instead of importing import { schema } .... you would simply import schema from ...

};

export default schema;
const schema = schemaDefinition as JSONSchema4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to define the variable as having JSONSchema4 type in the first place, rather than coercing it here with as. Any issue doing it that way?

const schemaDefinition: JSONSchema4 = {
  // ...
};

Copy link
Contributor Author

@yavorona yavorona Jul 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mjc1283 I agree, but I have tried it this way, and it complains Type '{ projectId: { type: "string"; required: true; }; accountId: ... ... }] is not assignable to type '{ [k: string]: JSONSchema4; }'. 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, I will create additional Jira tasks to fix/improve json schema validation and to document all the changes that we should do. I left const schema = schemaDefinition as JSONSchema4 so that all our unit tests can still pass without having to modify any of json schema files. FYI @mjc1283 @mikeng13

* @return {boolean} true if the given object is valid
*/
export var validate = function(jsonObject) {
export function validate(jsonObject: JSON): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JSON isn't the right type for this. This JSON is the global JSON object with methods parse and stringify. See here.

I think the right type for this is {}, an empty object with no properties. This is what's expected as the jsonSchemaValidate function's first argument.

Copy link
Contributor Author

@yavorona yavorona Jul 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mjc1283 Here is the eslint error from using {} (which is not intuitive at all IMO):

error  Don't use `{}` as a type. `{}` actually means "any non-nullish value".
- If you want a type meaning "any object", you probably want `Record<string, unknown>` instead.
- If you want a type meaning "any value", you probably want `unknown` instead  @typescript-eslint/ban-types

Looks like Omit<Record<any, never>, keyof any> can be used for explicitly empty object or something less strict like Record<string, never> . Any thoughts @mjc1283?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yea, this is forbidden by our lint rules. Actually I'm not sure why - I'm going to research it. Let's change it to unknown.

@yavorona yavorona force-pushed the pnguen/json-schema-validator-to-ts branch from a098778 to b64381b Compare July 28, 2020 23:37
@yavorona yavorona removed their assignment Jul 28, 2020
@yavorona yavorona force-pushed the pnguen/json-schema-validator-to-ts branch from b64381b to 1bc1d7f Compare July 29, 2020 16:49
@yavorona yavorona force-pushed the pnguen/json-schema-validator-to-ts branch from 1bc1d7f to fa3e204 Compare July 29, 2020 16:51
Copy link
Contributor

@mikeproeng37 mikeproeng37 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@yavorona yavorona merged commit de4e5b1 into mcarroll/optimizely-sdk-typescript Jul 30, 2020
@yavorona yavorona deleted the pnguen/json-schema-validator-to-ts branch July 30, 2020 00:05
yavorona added a commit that referenced this pull request Aug 1, 2020
… to TS (#524)

* Convert project_config_schema and json_schema_validator modules to TS

* Address Mike's comments

* Update validate function to accept empty object
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants