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

[Breaking change]: field.length is a string inside typeCast. #1426

Closed
felixmosh opened this issue Oct 17, 2021 · 9 comments
Closed

[Breaking change]: field.length is a string inside typeCast. #1426

felixmosh opened this issue Oct 17, 2021 · 9 comments

Comments

@felixmosh
Copy link

felixmosh commented Oct 17, 2021

Since v2.3.2 the field.length property that passed into typeCast function is a string instead of number!

Therefore it breaks boolean conversions.

typeCast: (field, next) => {
  if (field.type === 'TINY' && field.length === 1) {
  // --------------------------------------^ this check is failing, and it breaks the app (no booleans).
    return field.string() === '1'; // 1 = true, 0 = false
  }
  return next();
}

If this change was intentional, it is a breaking change, therefore I should be bumped in a major version.
I'm using knex query builder.

@sidorares
Copy link
Owner

@testn might be another regression related to #1402

@testn
Copy link
Contributor

testn commented Oct 17, 2021

let me take a look.

@sidorares
Copy link
Owner

I'm surprised how many users depend on typeCast, was going to deprecate it relatively soon in favour of "value mapping" option higher in the stack ( e.i after all deserealisation is already done )

@sidorares
Copy link
Owner

@felixmosh could you test by installing from master?

@testn
Copy link
Contributor

testn commented Oct 17, 2021

I'm surprised how many users depend on typeCast, was going to deprecate it relatively soon in favour of "value mapping" option higher in the stack ( e.i after all deserealisation is already done )

I agree. But you might need to come up with the specification?

@sidorares
Copy link
Owner

yeah, I'll try to create RFC with proposed spec

@felixmosh
Copy link
Author

@felixmosh could you test by installing from master?

Looks OK to me.

@testn
Copy link
Contributor

testn commented Oct 18, 2021

@felixmosh, let's close this?

@jclvicerra
Copy link

Seems already fixed in the release 2.3.3

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

No branches or pull requests

4 participants