-
Notifications
You must be signed in to change notification settings - Fork 938
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
Use sync promises #128
Use sync promises #128
Conversation
To be able to run the test independent of the linting.
A stricter eslint config should improve overall code consistency and make it easier for more people to contribute code that matches a consistent style.
For better compatibility and more consistency.
For better compatibility and consistency. ``` schema.validate x 5,495 ops/sec ±5.83% (61 runs sampled) schema.cast x 6,766 ops/sec ±3.26% (76 runs sampled) ```
hey there, thanks for your work. I'm not sure what to do with this because: its 95% none-relevant code changes which make this extremely hard to review. On the content of the PR i'm not sure what is being added or tweaked over #128 the original plan was to test out and validate the work that is already done works well, so I'm not sure why whole other attempt at it is needed? Thanks gain, let me know what your thinking |
d2812de
to
ffe29bb
Compare
Unfortunately sync validation performs much poorer than promise-based async validation: ``` schema.validate x 5,578 ops/sec ±6.31% (63 runs sampled) schema.validateSync x 2,898 ops/sec ±5.10% (81 runs sampled) schema.cast x 8,955 ops/sec ±0.63% (92 runs sampled) ```
3676f6d
to
f3f370d
Compare
@jquense as I've mentioned in the PR description this branch is based off #127. So in fact the actual change that introduces sync validation is in f3f370d which should be pretty straightforward to review. The rest are preparations/cleanups/simplifications, mostly contained in #127 already. I found it pretty hard and frustrating to rebase the rather ancient branches that already worked towards sync validation which is why I decided to do some cleanups and start over with sync validation from scratch. After all, once all the refactorings were in place, it only took an hour or so to get it done. I want to use this library in a production project but it turned out that I needed some features which are currently not present (like sync validation). In the course of course of implementing sync validation I realized that I found some parts of the code hard to read or understand which is why I started to refactor some parts since I assume I will have to do more changes to the code base in the future to suite my needs. From your comments in #127 I see that you're a bit reluctant to merge these changes around tooling and linting. Since you're the author of this library it's of course totally up to you to decide on how to deal with the evolution of the library. As outlined above I, as a user of the library, realized that the library gave me a really great foundation for what I need but that I have to develop substantial changes. I'm happy to contribute all these changes back, but I do not feel comfortable of doing so without proper test-tooling and style guides which reflect a widely accepted common sense rather than the personal opinion of the libraries original author. If introducing a stricter style guide and allowing refactorings like using more of lodash is a blocker for you then I'm happy with simply maintaining my own fork. If it's not a blocker for you, I'm happy to contribute all my changes back to this repository. |
I've already outlined in #127 that i don't find airbnb's style guide helpful or necessary. Adding it would only change superficial code style issues, and not reduce bugs, as the current config already has all the relevant none-style rules enabled, (similar to create react app). In terms of testing, you can see that yup is extensively tested so i'm not sure what where that concern is coming from. Beyond that, PR's should be focused and small so they don't accidentally introduce bugs and are easy to review. For example your replacements with lodash methods fundamentally change behavior in a few places (isPlainObject for my isObject) and other places add extra code with no or extremely minimal value, e.g The vast bulk of the work of the work you want done is already complete in #94 feature-wise. If you want that feature to be merged i would suggest just starting there, building and downloading the package from github and seeing if it works, and save yourself a ton of time fiddling around with the code style. In terms of the look of the code, if that is really a blocker for you using or contributing a library that is certainly fine, I'm not going to drastically change the code style or do things like "use more of lodash" with good reasons. You are of course welcome to maintain your own fork, that's gonna be a lot more work and time for you than needed, but you should certainly do what you feel is best for your project |
Based on #127.
This is a new take on sync validation. It uses https://github.com/fluffynuts/synchronous-promise as a drop-in replacement for native promises.
Unfortunately sync validation currently performs much worse than async validation (which indicates that something is still pretty wrong), but I'm sure this can be fixed.