PassFort / json-schema-to-flow-type

69 stars 17 forks source link

Handle OneOf With Properties #13

Closed wmonk closed 6 years ago

wmonk commented 6 years ago

Hi, thanks for this great tool! I hope you don't mind this unsolicited PR, but we are having to patch it at work, and thought instead I'd just try and fix it 😄.

Currently if you have a type like:

  {
    "inheritance": {
      "type": "object",
      "properties": {
        "key1": {
          "type": "string"
        },
        "key2": {
          "type": "string"
        }
      },
      "oneOf": [
        {
          "type": "object",
          "properties": {
            "key3": "string"
          }
        },
        {
          "type": "object",
          "properties": {
            "key4": "string"
          }
        }
      ],
      "required": [
        "key1",
        "key2"
      ]
    }
  }

it creates a type like:

type Inheritance = 
    | {
        key3: string
      }
    | {
        key4: string
      }

forgetting the other required properties that are "inherited". This fixes that, with a test.

dannynelson commented 6 years ago

Thanks @wmonk. I'll investigate why tests are failing and merge.

Actually, would you be interested in adopting this module? My company isn't using it anymore, so we have limited bandwidth to maintain it.

dannynelson commented 6 years ago

Published in json-schema-to-flow-type@0.2.7

wmonk commented 6 years ago

Brilliant! 😄

Yes we are using this at work, so could definitely look to take over 👍

wmonk commented 6 years ago

@dannynelson we'd definitely be up for taking over ownership of the project. We've already started doing some of the work to update babel dependencies.

What is the process for transferring?

dannynelson commented 6 years ago

@wmonk Do you want me to transfer it to your user? If so, you need to delete wmonk/json-schema-to-flow-type. Once you confirm the repo transfer, I will add you as an npm user.

wmonk commented 6 years ago

@dannynelson can you transfer to the @passfort organisation. The npm user would be passfort-dev

dannynelson commented 6 years ago

@wmonk it says I don't have permission to transfer to PassFort. How about I transfer to you then you transfer it to PassFort?

wmonk commented 6 years ago

Ah ok, it should be good to transfer to me now 😄

wmonk commented 6 years ago

@dannynelson sorry to bug, looking to get the latest babel updates in so we can take advantage of fixed-width types 😄

dannynelson commented 6 years ago

@wmonk I sent a transfer request to wmonk yesterday. Did you not receive an email?

wmonk commented 6 years ago

Ah damn just saw it, it's expired now thought! Ready and waiting whenever you can :)

dannynelson commented 6 years ago

@wmonk oops, ok. Sent another transfer request :)

wmonk commented 6 years ago

Brilliant, thanks! We’ve got some good stuff we want to extend, thanks for your hard work in getting this so far already!

On 20 Mar 2018, 17:55 +0000, Danny Nelson notifications@github.com, wrote:

@wmonk oops, ok. Sent another transfer request :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

dannynelson commented 6 years ago

@wmonk Great! Looking forward to seeing your changes.

FYI, I'm seeing errors when adding passfort-dev as an npm owner. I opened an issue on the npm registry. If they don't respond by Wednesday, I'll contact support directly.

dannynelson commented 6 years ago

@wmonk FYI, npm support resolved the issue and I added passfort-dev as an npm owner.