fix: remove array items type checking

[ivan-tymoshenko]

Type checking in this place is not only unnecessary (FJS is not a validator), it blocks type coercion for array items for no reason.

[Eomm]

Is patternProperties still a thing?

[ivan-tymoshenko]

I don't know why there is a patternProperties in this test. It works without them ('invalid' string always doesn't match the object schema). The test will work differently after this PR, but not because of patternProperties.

How the test works now:

test('invalid items throw', (t) => {
  t.plan(1)
  const schema = {
    type: 'object',
    properties: {
      args: {
        type: 'array',
        items: [
          {
            type: 'object',
            patternProperties: {
              '.*': {
                type: 'string'
              }
            }
          }
        ]
      }
    }
  }
  const stringify = build(schema)
  t.throws(() => stringify({ args: ['invalid'] })) // throws an error because 'invalid' is not an object
})

How it will work:

test('invalid items throw', (t) => {
  t.plan(1)
  const schema = {
    type: 'object',
    properties: {
      args: {
        type: 'array',
        items: [
          {
            type: 'object',
            patternProperties: {
              '.*': {
                type: 'string'
              }
            }
          }
        ]
      }
    }
  }
  const stringify = build(schema)
  t.equal(stringify({ args: ['invalid'] }), '{"args":[{"0":"i","1":"n","2":"v","3":"a","4":"l","5":"i","6":"d"}]}') // it coerces 'invalid' string to the object schema
})

It will work just like the same schema with patternProperties works now without array.

test('invalid items throw', (t) => {
  t.plan(1)
  const schema = {
    type: 'object',
    patternProperties: {
      '.*': {
        type: 'string'
      }
    }
  }
  const stringify = build(schema)
  t.equal(stringify('invalid'), '{"0":"i","1":"n","2":"v","3":"a","4":"l","5":"i","6":"d"}')
})

[ivan-tymoshenko]

@Eomm I decided to remove the test instead of updating it because this coercion looks like nonsense. If you want I can add the test like that.

[Eomm]

I like the idea of removing all the "validation" logic here, but I would move it to the next branch.

I think the new output could warn a user that does not understand what is going on: I would prefer an empty output instead of {"0":"i","1":"n","2":"v","3":"a","4":"l","5":"i","6":"d"}, but we can't do it without keeping the "validation" logic tho

[ivan-tymoshenko]

The output you see isn't related to the array at all. It's all about how FJS coerces string to object.

[climba03003]

I have merged master to next branch.
This PR should be target to next.

[mcollina]

I'm ok for landing this in the next branch.

[ivan-tymoshenko]

For me, it's more like a fix, but if you all think it should be merged into the next, I'm ok with it. I want to clarify, why did we merge #511 into the master branch instead of the next branch? It has the same type of changes.

[mcollina]

Ah ok! Then I'm ok with a fix/

[climba03003]

For me, it's more like a fix, but if you all think it should be merged into the next, I'm ok with it. I want to clarify, why did we merge #511 into the master branch instead of the next branch? It has the same type of changes.

It is hard to tell.
For me, if the PR is adding more supporting feature and do not break the current test (test change due to the feature is acceptable). It should be safe to land in master.

And here, we know that it will certainly break most of the people. It is more appropriate to wait for a major bump, not one PR for one major bump.
For example, #520, #504 (update on debug mode output) can be group inside next branch.

[ivan-tymoshenko]

As I said, I'm ok with it.

P.S. #504 is completely safe to release.