Extend schema fixes #65

[aboutlo]

Better merge strategy. Before, the merge was replacing the array element rather than merging the items.
I decided to block all the method after .extend to avoid incorrect usage.

Wrong but no error raised by FluentSchema

const SCHEMA_BASE = S.object()
  .prop(
    'reason',
    S.string()
      .title('Reason 1')
  )

const base = S.object()
  .extend(SCHEMA_BASE)
  .prop(
    'reason',
    S.string()
      .minLength(1)
  )
  

Correct usage should be:

const extended = S.object()
  .prop(
    'reason',
    S.string()
      .minLength(1)
  )
  .extend(SCHEMA_BASE)

First, the schema is defined with all the props then it extends the base schema.

With the sealed schema after the .extend the only thing that can be done is calling valueOf

I'm unsure if I should mark this as 1.0.2 because it's a bug fix or considering the sealing as breaking change. Feedback welcome.

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows Code of conduct

[aboutlo]

I'm unsure if I should mark this as 1.0.2 because it's a bug fix or considering the sealing as breaking change. Feedback welcome.

@mcollina and @Eomm any strong opinion?

[mcollina]

Looks like a bug to me.. how high is the chance of some code breaking because of this change?

[aboutlo]

If you look the bug reported the snippet provided is broken after the upgrade #65.

const NEW_SCHEMA = S.object()
  .extend(SCHEMA_BASE)
  .prop(
    'reason',
    S.string()
      .minLength(1)
  )

.prop is added after .extend

[mcollina]

Mark it as 1.0.2. I don't think the library is really usable without this fix.

[jmav]

This does not work with multiple extend, and I think it could fail on some cases users are using.

const SCHEMA_V1 = S.object()
  .prop(
    'reason',
    S.string()
      .minLength(1)
  )
  .extend(SCHEMA_BASE)

const SCHEMA_NEW = S.object()
  .prop(
    'reason',
    S.string()
      .minLength(1)
  )
  .extend(SCHEMA_V1)

[aboutlo]

Tks for raise this. Can you add the expected result?

On Wed, 20 May 2020 at 09:51, Jure ***@***.***> wrote: This does not work with multiple extend, and I think it could fail on some cases users are using. const SCHEMA_V1 = S.object() .prop( 'reason', S.string() .minLength(1) ) .extend(SCHEMA_BASE) const SCHEMA_NEW = S.object() .prop( 'reason', S.string() .minLength(1) ) .extend(SCHEMA_V1) — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#69 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJN2LHNCROMXTMJBPGX5FDRSOKYHANCNFSM4M4O726A> .

-- Sent from mobile phone

[jmav]

Expected behavior:
Props should be allowed to be extended multiple times.

SCHEMA_V1 should have all definitions defined in SCHEMA_BASE + prop "reason"
SCHEMA_NEW should have all definitions defined in SCHEMA_V1 and in SCHEMA_BASE + change