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

Major Refactors #92

Open
1 of 6 tasks
prust opened this issue Oct 31, 2016 · 6 comments
Open
1 of 6 tasks

Major Refactors #92

prust opened this issue Oct 31, 2016 · 6 comments
Milestone

Comments

@prust
Copy link
Collaborator

prust commented Oct 31, 2016

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.

  • Make SqlBricks namespace a function or class, so you have to call SqlBricks().select instead of SqlBricks.select (or var select = require('sql-bricks')().select;). This way you can set the default options, like SqlBricks({placeholder: '?%d'}) (Conflict with pg-promise #91) and that way extensions won't need to pollute the global namespace to change things like sql.conversions (Need a way to add new converters #62).
  • replace the _extension/subclass/inherits/applyNew stuff with basic prototypical inheritance & restrict the way SQLBricks can be called (throw an error if it's called with or without new, depending on whether it's required)
  • remove the defineClause/mini-templating language Replace custom templating with ES2015 template literals #100
  • remove the placeholder-replacing code
  • reduce the variety of ways things can be called (values as an array, object vs separate args, insert() vs insert.into() vs split_keys_vals_mode). Have one right way, thus reducing the API surface area & making the lib shorter, simpler & more maintainable
  • (possibly) support named parameters (Conflict with pg-promise #91)

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).

@prust prust changed the title Major Refactor 3.0 Refactor Oct 31, 2016
@prust prust added this to the 3.0 milestone Aug 6, 2017
@paleo
Copy link

paleo commented May 5, 2018

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

@prust
Copy link
Collaborator Author

prust commented May 7, 2018

@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
Copy link

paleo commented May 8, 2018

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
Copy link
Collaborator Author

prust commented Oct 20, 2018

@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
Copy link

paleo commented Dec 14, 2018

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
Copy link
Collaborator Author

prust commented Oct 23, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants