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.

Sign up for GitHub

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

Jump to bottom

string_decoder: do not declare as a class #18723

Closed
wants to merge 2 commits into from
Closed

string_decoder: do not declare as a class #18723

wants to merge 2 commits into from

Conversation

apapirovski
Copy link
Member

@apapirovski apapirovski commented Feb 12, 2018 •
edited
Loading

There are libraries which invoke StringDecoder using .call and .inherits, which directly conflicts with making StringDecoder be a class which can only be invoked with the new keyword. Revert to declaring it as a function with prototype props.

This fixes the iconv-lite package: https://github.com/ashtuchkin/iconv-lite which is relied on by https://github.com/expressjs/body-parser and https://github.com/expressjs/express within CitGM

Refs: #18537

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

string_decoder

@apapirovski apapirovski added string_decoder Issues and PRs related to the string_decoder subsystem. fast-track PRs that do not need to wait for 48 hours to land. labels Feb 12, 2018
@apapirovski apapirovski requested a review from addaleax February 12, 2018 06:24
@nodejs-github-bot nodejs-github-bot added the string_decoder Issues and PRs related to the string_decoder subsystem. label Feb 12, 2018
@apapirovski
string_decoder: do not declare as a class
b7d6315 
There are libraries which invoke StringDecoder using .call
and .inherits, which directly conflicts with making
StringDecoder be a class which can only be invoked with
the new keyword. Revert to declaring it as a function.
@apapirovski apapirovski force-pushed the fix-string-decoder-regression branch from f9b677d to b7d6315 Compare February 12, 2018 06:25
@apapirovski
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/13114/
CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1279/

@daynin
Copy link
Contributor

daynin commented Feb 12, 2018

@apapirovski maybe we should deprecate StringDecoder as a function? What do you think?

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 12, 2018
addaleax
addaleax approved these changes Feb 12, 2018
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Thanks for catching this!

@apapirovski
Copy link
Member Author

maybe we should deprecate StringDecoder as a function? What do you think?

I don't have a strong opinion but I'm leaning towards 'no' because there's no maintenance burden (classes are a little nicer syntax but mostly there's no substantial difference) and because the fact that express is the biggest consumer makes it extremely unlikely that we'll ever be able to go beyond a doc-deprecation. (As far as I can tell, iconv-lite is also used by grunt and a few other big packages...)

@apapirovski
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/13126/

@apapirovski apapirovski force-pushed the fix-string-decoder-regression branch from cae1b04 to 269a3dc Compare February 12, 2018 21:47
addaleax
addaleax approved these changes Feb 12, 2018
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Still LGTM! :)

@apapirovski
Copy link
Member Author

Landed in e782715

@apapirovski apapirovski deleted the fix-string-decoder-regression branch February 13, 2018 13:03
apapirovski added a commit that referenced this pull request Feb 13, 2018
@apapirovski
string_decoder: fix regressions
e782715 
There are libraries which invoke StringDecoder using .call and
.inherits, which directly conflicts with making StringDecoder
be a class which can only be invoked with the new keyword.
Revert to declaring it as a function.

StringDecoder#lastNeed was not defined, redefine it using
the new interface and fix StringDecoder#lastTotal.

PR-URL: #18723
Refs: #18537
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
@targos targos mentioned this pull request Feb 14, 2018
Closed
@devsnek
Copy link
Member

devsnek commented Feb 18, 2018

for the future, would using internal/util's createClassWrapper have been another way to fix a regression like this?

@addaleax
Copy link
Member

@devsnek Yes … it’s worth mentioning though that that still comes with a noticeable perf hit, and the original PR already traded a bit of initialization performance for decoding performance, so using createClassWrapper would probably add to that.

@MylesBorins
Copy link
Contributor

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018
@apapirovski @BridgeAR
string_decoder: fix regressions
181d522 
There are libraries which invoke StringDecoder using .call and
.inherits, which directly conflicts with making StringDecoder
be a class which can only be invoked with the new keyword.
Revert to declaring it as a function.

StringDecoder#lastNeed was not defined, redefine it using
the new interface and fix StringDecoder#lastTotal.

PR-URL: nodejs#18723
Refs: nodejs#18537
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@apapirovski @MayaLekova
string_decoder: fix regressions
5783e33 
There are libraries which invoke StringDecoder using .call and
.inherits, which directly conflicts with making StringDecoder
be a class which can only be invoked with the new keyword.
Revert to declaring it as a function.

StringDecoder#lastNeed was not defined, redefine it using
the new interface and fix StringDecoder#lastTotal.

PR-URL: nodejs#18723
Refs: nodejs#18537
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@cjihrig cjihrig cjihrig approved these changes

@targos targos targos approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@daynin daynin daynin approved these changes

@evanlucas evanlucas evanlucas approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. string_decoder Issues and PRs related to the string_decoder subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.