CSNW / sql-bricks

Transparent, Schemaless SQL Generation
http://csnw.github.io/sql-bricks
MIT License
203 stars 25 forks source link

Major Refactors #92

Open prust opened 7 years ago

prust commented 7 years ago

In order to address a variety of small issues, I think sql-bricks needs a significant refactor. I want to reduce the line count, but more importantly, I want to reduce the complexity. SqlBricks is no longer as "easy to understand & debug" as it claims to be.

While we're at it, we might as well fix the outstanding bugs & issues, but I don't want those to necessarily slow down the refactoring. It might be better to split the refactoring into pieces and do them separately over time, but I'd like to release it as one big 3.x rather than multiple backwards-incompatible releases.

Oct 2017 Update:

Looking through the capabilities and the code, I think there must be a cleaner, more generic way to go about this. I suspect that the important parts of this library can be implemented in 100 or 200 lines of code instead of ~1k. Drop the "mini" templating language and the subclassing/inherits. Probably be a little more rigid in what kinds of input the methods accept. Use JSON to define all the clauses (and their order) and what kinds of input they accept. It may make sense to template strings, but if so it should be a one-line regex, not a mini-templating language. Allow extension in a simpler way: by allowing consumers/libraries to directly swap out methods or metadata, like Backbone.ajax/sync. The toParams() parameter substitution code is quite hairy (auto-numbering, etc); we probably need to tighten up the scope and disallow certain things there.

Mar 2022 Update:

I renamed this Issue from "3.0 Refactor" to "Major Refactors", since I decided to go ahead and ship v3.0 with only one of these (remove the defineClause/mini-templating language #100), along with the removal of the Underscore.js dependency. I would still like to tackle some of these, especially the Oct 2017 idea of simplifying the library dramatically, but I don't have much motivation to do so, since I don't use this library often (in most cases I think it's better to write the SQL directly).

paleo commented 6 years ago

Could you consider switching to TypeScript and to a modern JS instead of ES5 / Underscore?

prust commented 6 years ago

@paleo: Thanks for the input.

Could you consider switching to TypeScript

This is the 2nd request for TypeScript; I'll probably add an optional TypeScript declaration file (*.d.ts) at some point and keep it updated so that those who use TypeScript can have the convenience of SQLBricks type definitions in their projects. I'm not sure if it'll be in 3.0 (it won't have to be in a major release, since it won't break backwards compatibility), but the more requests I get for it, the sooner I'll do it. Of course, a pull request with this would be welcome & would save me the work. That said, I'm not interested in rewriting SQLBricks with embedded TypeScript types, so I wouldn't merge a pull request for that.

to a modern JS instead of ES5 / Underscore?

I am planning to remove the Underscore dependency by replacing the Underscore utility functions (_.map(), _.isArray(), etc) with their native JS equivalents (whether from ES5 or from more "modern" versions) and the first 3.0 beta does already utilize ES2015 Template Literals. That said, I don't plan to chase every new syntax or feature that ECMAScript adds each year unless there is a tangible benefit that I believe outweighs the cost, and I am very reticent to adopt syntax/features that are not widely supported and therefore require transpilation.

To be specific, at present I'm not planning to utilize the new class syntax, arrow functions or modules in SQLBricks 3.0. I am open to being argued away from any of those decisions if there is a compelling argument, but I'm not interested in migrating the codebase to utilize every new ECMAScript syntax/feature just because it's been included in the standard.

paleo commented 6 years ago

I'll probably add an optional TypeScript declaration file (*.d.ts) at some point and keep it updated so that those who use TypeScript can have the convenience of SQLBricks type definitions in their projects.

Fyi, one of the guys on my team and I published a TypeScript definitions file for SQLBricks on DefinitelyTyped. It is still not complete (some missing types are replaced by any). You can take the file index.d.ts, include it in your npm package, then remove it from DefinitelyTyped. I can help you for that, or submit a pull request. But if you publish the types in your package, it's up to you to make that new implementation-side features are reflected in the definitions.

I am very reticent to adopt syntax/features that are not widely supported and therefore require transpilation

Since Node 8, transpilation is not required for ES2015, ES2016 and most of ES2017 features.

Except for modules: if we want to bundle our library we can use ES6 module syntax then use Rollup. But I'm unsure we could name the bundler work a "transpilation".

I'm not planning to utilize the new class syntax, arrow functions or modules in SQLBricks 3.0.

That was the reason of my question. I could have helped you to refactore the big unique file that uses old fashion Statement.prototype._add = function ... etc., into several modules, arrow functions and classes. ;)

prust commented 5 years ago

@paleo: I'm sorry for the long delay responding to this, I had turned off GitHub notifications b/c I was already getting notifications for a lot of repos in Slack, but didn't realize I wasn't getting notifications on this repository. This is fixed now.

I can help you for that, or submit a pull request.

If you're still willing to submit a pull request for this, I would appreciate it.

But if you publish the types in your package, it's up to you to make that new implementation-side features are reflected in the definitions.

Understood, I don't think it'll be too much of a burden to keep the types maintained, so I think I can commit to keeping them updated, at least for the 2.x and 3.x major versions.

paleo commented 5 years ago

If you're still willing to submit a pull request for this, I would appreciate it.

Done.

In case this is useful, I develop a lib that, in conjunction with SQL Bricks, offers a solid alternative to Knex.

prust commented 4 years ago

@paleo: FYI, I added a reference to "A Layer Above Database Connectors" to the readme in 9663defe67 -- feel free to issue a pull request if you'd like to update it in any way.