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.
> the solution is not good as it will result in 'a' to match instead of 'aaa' as 'a' included in 'a' but not equal to.
Not really, it won't.
You may be confusing Array#includes and String#includes or something, but just try it in the console:
> ['aaa', 'aa'].includes('a')
false
The final part of the article implies editableNoteTypes is still an array, which still works as expected:
> let editableNoteTypes = ['aaa', 'aa'];
> editableNoteTypes.includes('a')
false
---
Still, I do agree that the proposed solution is not good. Using Array#includes does not really improve readability. And to boot it only applies to a very specific conditional where you're doing exactly "x === a || x === b || x ==== c" but no other kind of combination.
Instead, if you do want to improve readability, you should give that check a name. Put it into a function and then use that function.
function isEditableNote(type) {
// whatever...
}
// then...
if (isEditableNote(noteType)) {
// ...
}
> But lego-build does more than help you make front-end components, tools that do that already exist. Lego-build is far more powerful.
This is your claim. And yet, both the article and the actual lego-build website *only* show doing "lego-build component Nav". I mean, there are a lot of words written, sure, but the only example ever shown is that one. Nothing else.
I'm not saying this as a criticism to the tool itself, but mostly as a criticism of your marketing skills. Showing the simplest thing which other tools can already do, and then claiming that your tool can do a lot more but *not* showing that, doesn't make your tool sound interesting. It makes it sound like snake oil.
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.> the solution is not good as it will result in 'a' to match instead of 'aaa' as 'a' included in 'a' but not equal to. Not really, it won't. You may be confusing Array#includes and String#includes or something, but just try it in the console: > ['aaa', 'aa'].includes('a') false The final part of the article implies editableNoteTypes is still an array, which still works as expected: > let editableNoteTypes = ['aaa', 'aa']; > editableNoteTypes.includes('a') false --- Still, I do agree that the proposed solution is not good. Using Array#includes does not really improve readability. And to boot it only applies to a very specific conditional where you're doing exactly "x === a || x === b || x ==== c" but no other kind of combination. Instead, if you do want to improve readability, you should give that check a name. Put it into a function and then use that function. function isEditableNote(type) { // whatever... } // then... if (isEditableNote(noteType)) { // ... }