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

Template literals function for use with callbacks #960

Merged
merged 2 commits into from
Dec 27, 2019

Conversation

bessiambre
Copy link

@bessiambre bessiambre commented Dec 12, 2019

fixes #957

This allows doing this:

let hw='hello world';
let request=new mssql.Request(pool);
request.query(request.template`SELECT ${hw}`,(err,result)=>{
    if(err){done(err); return;}
    console.log(result.recordset);
});

Copy link
Collaborator

@dhensby dhensby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some questions, I'm not entirely clear on how the callbacks are being accepted / executed in the template method.

lib/base/request.js Show resolved Hide resolved
test/common/unit.js Show resolved Hide resolved
@dhensby
Copy link
Collaborator

dhensby commented Dec 13, 2019

OK - I think I'm seeing a bit better what's going on here...

is the problem that you can't do:

request.query`some templated query`, cb

because there's no way to pass the cb option?

@willmorgan what are your thoughts on this?

@dhensby
Copy link
Collaborator

dhensby commented Dec 13, 2019

As I've got a bit of a better understanding of this, I'd think that the templating functions should probably be moved to utils rather than being part of the Request class

@dhensby dhensby requested a review from willmorgan December 13, 2019 12:17
Copy link
Collaborator

@willmorgan willmorgan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're doing tagged templates then why are we regressing from ES6 and doing callbacks?

Why not use async/await?

await request.query`select stuff from theplace`

Copy link
Collaborator

@willmorgan willmorgan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not fully sure if this is needed given above comment, but if I'm convinced, it needs to go into util as it has no business being on a request object.

@bessiambre
Copy link
Author

There is still a lot of us trying to avoid promises as they are problematic:
https://medium.com/@b.essiambre/continuation-passing-style-patterns-for-javascript-5528449d3070?source=friends_link&sk=976fb25ca6c15eba3a4badcf55ba698e

As for putting this in a 'utils' class, I don't think this would work since the template literal function needs to add input parameters to the request object.

@dhensby
Copy link
Collaborator

dhensby commented Dec 13, 2019

All good points, OK - I think I'm convinced - @willmorgan ?

@willmorgan
Copy link
Collaborator

I'm not convinced at all if I'm honest. The rest of the library is async and in 5 years of writing async Node code, I've not suffered due to choosing a promise library that swallows exceptions.

@bessiambre
Copy link
Author

There is a Callbacks section in the readme, there is no indication that this is deprecated? Lot's of people find promises unnecessary and over engineered. I don't like to wrap all my async functions into stateful objects.

@bessiambre
Copy link
Author

Is there a tweak to this PR that would make this more palatable for you @willmorgan? I don't have a strong opinion on the naming, on other details or even on structuring this differently. I find template literals for SQL to be a great idea. Including this three statement function allows me to use them without monkeypatching.

@willmorgan
Copy link
Collaborator

Not really, but thanks for asking 😄

I don't agree with the reasoning for callbacks in this particular use case, don't believe that furthering support of callbacks are the way forward with any modern JS library, and generally do not feel great about supporting yet another usage method.

However for completeness and consistency, maybe this should be taken in.

I would, however, move to deprecate callbacks in a later major release.

@bessiambre
Copy link
Author

Is anything holding this up?

@dhensby
Copy link
Collaborator

dhensby commented Dec 19, 2019

I need to find some time to be certain about this, I'm not entirely sure we can't use a util helper and I'll take a look over it in the coming days

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

Successfully merging this pull request may close these issues.

Template literals with callbacks
3 participants