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

Es6fy #1465

Closed
wants to merge 2 commits into from
Closed

Es6fy #1465

wants to merge 2 commits into from

Conversation

billouboq
Copy link
Contributor

@billouboq billouboq commented Oct 1, 2017

Change all objects with custom prototypes with ES6 classes and remove self variables by using arrow functions.

If you want I can change var with let-const too

I think the only breaking change is that you can't use query without new anymore :

if (!(this instanceof Query)) { return new Query(config, values, callback) }

@charmander
Copy link
Collaborator

To avoid breaking changes, maybe it’s best to only include the arrow function commit for now?

charmander added a commit to charmander/node-postgres that referenced this pull request Oct 16, 2017
…sts with npm 5

Should save some confusion in future pull requests (brianc#1465, brianc#1436, brianc#1363).
@billouboq
Copy link
Contributor Author

Since all tests pass, don't know if it's a use case and if people are using it alone

@charmander
Copy link
Collaborator

People are constructing Query objects explicitly. It may be that they’re all using new, but I don’t think class syntax is worth risking that that’s not the case. It also breaks non-ES6-class inheritance of all the pg types.

This isn’t the sort of thing tests would cover.

@abenhamdine
Copy link
Contributor

abenhamdine commented Oct 16, 2017

IMHO @charmander is totally right, it would be much safer to include such a potential breaking change in a major version.
It's difficult and risky to guess actual usage by the ecosystem, notably for a module downloaded more than 1 million times a month.

@billouboq
Copy link
Contributor Author

@abenhamdine @charmander what about reverting only query class to function but keep the rest with es6 class ?

@charmander
Copy link
Collaborator

As I mentioned in the latest comment, it also stops people from being able to inherit from pg types without using class, so that would still be a breaking change.

@abenhamdine
Copy link
Contributor

@billouboq I think it would be better if the PR would be splitted in several ones :

  • use arrow functions
  • potential use const/let
  • use es6 class (semver major)
    It would ease the review and separate minor/major changes.
    Also brianc will have probably something to say about the es6 class stuff.

@billouboq
Copy link
Contributor Author

Ok will seperate them, thanks for the review

@billouboq billouboq closed this Oct 17, 2017
brianc pushed a commit that referenced this pull request Nov 4, 2017
…sts with npm 5

Should save some confusion in future pull requests (#1465, #1436, #1363).
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.

None yet

3 participants