If you're open to listen to suggestions, I think there are a couple of small things that would be interesting to look into.
If you're not, then just ignore this comment :)
---
The main issue is related to this:
> The previous code can be written in the following "short way"
There are two ways of doing the same thing. That, in itself, seems unnecessary. But the problem is a bit more nuanced because:
- There's no explanation about any differences. Are the two ways 100% equivalent? Is one preferred over the other? Just having both without explanation will cause inconsistencies for the potential developers using your library and some extra work for yourself. But without explanation, these downsides seem unjustified.
- The string-based way is only available for built-ins but not for custom pipes. This immediately suggests anyone using custom pipes should avoid the string-based syntax for consistency in their code.
- The string-based syntax alone is not entirely consistent e.g. array is not included in the syntax.
- Then again, the function-based syntax is definitely clunkier with those [...()] you need to write every time. I mean, just look at the children property: [array({ type: [string()] })]. Ufff.
So... all in all, it isn't really clear which syntax would be the best one to use for a developer. And since there's no explanation on your side it also remains unclear whether *you* prefer one or the other and so could potentially discard one in the future.
I think it would be a good idea to solve this early, if you really want people to use the library and not be bitten by this problem later.
---
Another issue is the usage of "multiple schemas". This is only mentioned as a comment in the code of the example in the README. It says "these two pipes are applied in order" and "we can apply two pipes".
But, really, it's not just "two pipes", it's "multiple" and "schemas". You can apply two or more full schemas. In fact, schema is expected-and-if-not-coerced into being an array of schemas [ https://github.com/gchumillas/schema-fixer/blob/c5f1f43a6559b7f9363e8a3aafc4b8a6d159c1a1/src/index.js#L34 ].
The problem with this has two different aspects. On the one hand, it's similar to the above missing explanation: It is not really documented or explained. But on the other hand it's a bit subtler: why?
I mean, what is the use case for piping through multiple schemas? Usually what you'd do is compose the various pipes into one. That is, given string, trim, and lower, you would compose all those into a single "lowercase-trimmed-string" function and use that as the "precise type" you want the data to be. That way you first create custom pipes that are meaningful to your data and then apply them everywhere. Doing that, there's really no need for passing multiple pipes at all.
Hi Kali,
Thank you very much for taking the time to look at the library. I really appreciate it.
Answers below :)
> There are two ways of doing the same thing.
I should have explained that the "short way" is just a "sugar way" of writing the same things. The correct way is the "long way". But this point needs a better explanation.
Also, the "short way" is not as versatile as the "long way". You can do simple things, but no more. That is the intended use. I should explain this point too.
> I mean, what is the use case for piping through multiple schemas? Usually what you'd do is compose the various pipes into one.
You can create a pipe as the result of composing multiple pipes. And then apply that pipe to the desired fields.
But maybe you are going to use that pipe only once in your code. In that case I think that an array is better.
Thank you again for your comments. They are really helpful.
If you're open to listen to suggestions, I think there are a couple of small things that would be interesting to look into. If you're not, then just ignore this comment :) --- The main issue is related to this: > The previous code can be written in the following "short way" There are two ways of doing the same thing. That, in itself, seems unnecessary. But the problem is a bit more nuanced because: - There's no explanation about any differences. Are the two ways 100% equivalent? Is one preferred over the other? Just having both without explanation will cause inconsistencies for the potential developers using your library and some extra work for yourself. But without explanation, these downsides seem unjustified. - The string-based way is only available for built-ins but not for custom pipes. This immediately suggests anyone using custom pipes should avoid the string-based syntax for consistency in their code. - The string-based syntax alone is not entirely consistent e.g. array is not included in the syntax. - Then again, the function-based syntax is definitely clunkier with those [...()] you need to write every time. I mean, just look at the children property: [array({ type: [string()] })]. Ufff. So... all in all, it isn't really clear which syntax would be the best one to use for a developer. And since there's no explanation on your side it also remains unclear whether *you* prefer one or the other and so could potentially discard one in the future. I think it would be a good idea to solve this early, if you really want people to use the library and not be bitten by this problem later. --- Another issue is the usage of "multiple schemas". This is only mentioned as a comment in the code of the example in the README. It says "these two pipes are applied in order" and "we can apply two pipes". But, really, it's not just "two pipes", it's "multiple" and "schemas". You can apply two or more full schemas. In fact, schema is expected-and-if-not-coerced into being an array of schemas [ https://github.com/gchumillas/schema-fixer/blob/c5f1f43a6559b7f9363e8a3aafc4b8a6d159c1a1/src/index.js#L34 ]. The problem with this has two different aspects. On the one hand, it's similar to the above missing explanation: It is not really documented or explained. But on the other hand it's a bit subtler: why? I mean, what is the use case for piping through multiple schemas? Usually what you'd do is compose the various pipes into one. That is, given string, trim, and lower, you would compose all those into a single "lowercase-trimmed-string" function and use that as the "precise type" you want the data to be. That way you first create custom pipes that are meaningful to your data and then apply them everywhere. Doing that, there's really no need for passing multiple pipes at all.