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

feat: support built-in JavaScript/Node schema types #1731

Merged
merged 1 commit into from
Sep 24, 2018
Merged

Conversation

hacksparrow
Copy link
Contributor

@hacksparrow hacksparrow commented Sep 19, 2018

Adds support for Date type and includes test for pre-existing types.

For fixing: #1112

A new issue has been created for items under "nice to have" - #1742.

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

@hacksparrow hacksparrow self-assigned this Sep 19, 2018
@hacksparrow hacksparrow force-pushed the fix/#1112 branch 2 times, most recently from 75d2702 to 9d3f0cc Compare September 20, 2018 10:10
@hacksparrow hacksparrow changed the title [WIP] feat: support built-in JavaScript/Node schema types feat: support built-in JavaScript/Node schema types Sep 20, 2018
it('converts arrays', () => {
it('converts Date', () => {
expect(metaToJsonProperty({type: Date})).to.eql({
type: 'date',
Copy link
Member

Choose a reason for hiding this comment

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

I think JavaScript Date should be described as date-time, because Date holds both date and time.

https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.1.md#data-types

Common Name type format Comments
date string date As defined by full-date - RFC3339
dateTime string date-time As defined by date-time - RFC3339

LoopBack 3.x describes Date properties as {type: 'string', format: 'date-time'} too, see loopback-swagger's lib/specgen/schema-builder.js:

    case 'date':
      schema.type = 'string';
      schema.format = 'date-time';
      break;

@hacksparrow
Copy link
Contributor Author

@bajtos made the changes.

@@ -125,6 +126,10 @@ export function metaToJsonProperty(meta: PropertyDefinition): JSONSchema {

if (isComplexType(propertyType)) {
Object.assign(propDef, {$ref: `#/definitions/${propertyType.name}`});
} else if (propertyType === Date) {
Object.assign(propDef, {
type: 'date-time',
Copy link
Member

Choose a reason for hiding this comment

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

type: 'string', 
format: 'date-time',

Copy link
Member

Choose a reason for hiding this comment

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

@hacksparrow before you fix this: please look around the tests and try to find a test that runs JSON Schema validation against the JSON/OpenAPI schema we are producing.

Ideally, if our schema builder emits an invalid schema (as you did here), then there should be a test that fails and reports the problem.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we have an acceptance test that needs to be expanded with the newly supported types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted.

@hacksparrow hacksparrow force-pushed the fix/#1112 branch 4 times, most recently from 533ce5b to 775297f Compare September 21, 2018 13:59
@hacksparrow
Copy link
Contributor Author

@bajtos made the changes you suggested.

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Nice 👍

Please clean up the git history before merging.

@hacksparrow hacksparrow force-pushed the fix/#1112 branch 2 times, most recently from 3c993b7 to cbf977e Compare September 21, 2018 15:26
@raymondfeng
Copy link
Contributor

Please fix the lint issue before merging - https://travis-ci.org/strongloop/loopback-next/jobs/431549108

Add support for built-in JavaScript/Node schema types in Model
properties
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

Successfully merging this pull request may close these issues.

4 participants