GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — crypto: make LazyTransform compabile with Streams1 by mcollina · Pull Request #12380 · nodejs/node

Conversation

@mcollina

@mcollina mcollina commented Apr 13, 2017

Copy link
Copy Markdown
Member

Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: #12269

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)

crypto, streams

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mscdex mscdex added crypto Issues and PRs related to the crypto subsystem. stream Issues and PRs related to Node.js streams. labels Apr 13, 2017
@sam-github

sam-github commented Apr 13, 2017

Copy link
Copy Markdown
Contributor

s/compabile/compatible/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
cjihrig
cjihrig requested changes Apr 13, 2017
View reviewed changes
Comment thread test/parallel/test-crypto-lazy-transform-writable.js Outdated

@cjihrig cjihrig Apr 13, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can you change this to // Calculate the expected result.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread test/parallel/test-crypto-lazy-transform-writable.js Outdated

@cjihrig cjihrig Apr 13, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think the third parameter should say the hash doesn't match.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sam-github sam-github Apr 13, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Just my two bits, but I think most description strings are a waste of time and space. The reported on failure line number leads directly to the assert code, which is quite readable. As-is, the string just repeats the arg name and the word "match", which is a synonym for "equal", so its just a comment that restates exactly what the code says, a classic anti-pattern.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax and lpinca reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
jasnell
jasnell approved these changes Apr 13, 2017
View reviewed changes
addaleax
addaleax approved these changes Apr 13, 2017
View reviewed changes
@refack
refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:12
@mcollina
crypto: make LazyTransform compabile with Streams1
06844ff 
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: nodejs#12269
@mcollina
mcollina force-pushed the lazy-transform branch from b44b806 to 06844ff Compare April 14, 2017 08:42
@mcollina

mcollina commented Apr 14, 2017

Copy link
Copy Markdown
Member Author

@cjihrig @sam-github updated.

CI: https://ci.nodejs.org/job/node-test-pull-request/7397/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
cjihrig
cjihrig approved these changes Apr 14, 2017
View reviewed changes
@mcollina

mcollina commented Apr 14, 2017

Copy link
Copy Markdown
Member Author

Landed in 1ae172b
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina mcollina closed this Apr 14, 2017
@mcollina
mcollina deleted the lazy-transform branch April 14, 2017 14:17
mcollina added a commit that referenced this pull request Apr 14, 2017
@mcollina
crypto: make LazyTransform compabile with Streams1
1ae172b 
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: #12269
PR-URL: #12380
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
evanlucas pushed a commit that referenced this pull request Apr 25, 2017
@mcollina @evanlucas
crypto: make LazyTransform compabile with Streams1
ebc69be 
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: #12269
PR-URL: #12380
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@evanlucas evanlucas mentioned this pull request May 1, 2017
v7.10.0 proposal #12775
Merged
evanlucas pushed a commit that referenced this pull request May 1, 2017
@mcollina @evanlucas
crypto: make LazyTransform compabile with Streams1
18dbc1c 
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: #12269
PR-URL: #12380
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
evanlucas pushed a commit that referenced this pull request May 2, 2017
@mcollina @evanlucas
crypto: make LazyTransform compabile with Streams1
8ac8e50 
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: #12269
PR-URL: #12380
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins

MylesBorins commented May 15, 2017

Copy link
Copy Markdown
Contributor

@mcollina I've backported to v6.x-staging please lmk if you think this was a mistake and we can rebase it out.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
MylesBorins pushed a commit that referenced this pull request May 15, 2017
@mcollina @MylesBorins
crypto: make LazyTransform compabile with Streams1
b1dc123 
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: #12269
PR-URL: #12380
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins added land-on-v6.x notable-change PRs with changes that should be highlighted in changelogs. labels May 15, 2017
@mcollina

mcollina commented May 16, 2017

Copy link
Copy Markdown
Member Author

@MylesBorins you did well, it should be backported.
MylesBorins and gibfahn reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
MylesBorins pushed a commit that referenced this pull request May 18, 2017
@mcollina @MylesBorins
crypto: make LazyTransform compabile with Streams1
0a79b2d 
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: #12269
PR-URL: #12380
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 23, 2017
v6.11.0 proposal #13059
Merged
MylesBorins added a commit that referenced this pull request Jun 6, 2017
@MylesBorins
2017-06-06, Version 6.11.0 'Boron' (LTS)
40859e2 
This LTS release comes with 126 commits. This includes 40 which
are test related, 32 which are doc related, 12 which are
build / tool related and 4 commits which are updates to
dependencies.

Notable Changes:

* build:
  - support for building mips64el (nanxiongchao)
    #10991
* cluster:
  - disconnect() now returns a reference to the disconnected
    worker. (Sean Villars)
    #10019
* crypto:
  - ability to select cert store at runtime (Adam Majer)
    #8334
  - Use system CAs instead of using bundled ones (Adam Majer)
    #8334
  - The `Decipher` methods `setAuthTag()` and `setAAD` now return
    `this`. (Kirill Fomichev)
    #9398
  - adding support for OPENSSL_CONF again (Sam Roberts)
    #11006
  - make LazyTransform compabile with Streams1 (Matteo Collina)
    #12380
* deps:
  - upgrade libuv to 1.11.0 (cjihrig)
    #11094
  - upgrade libuv to 1.10.2 (cjihrig)
    #10717
  - upgrade libuv to 1.10.1 (cjihrig)
    #9647
  - upgrade libuv to 1.10.0 (cjihrig)
    #9267
* dns:
  - Implemented `{ttl: true}` for `resolve4()` and `resolve6()`
    (Ben Noordhuis)
    #9296
* process:
  - add NODE_NO_WARNINGS environment variable (cjihrig)
    #10842
* readline:
  - add option to stop duplicates in history (Danny Nemer)
    #2982
* src:
  - support "--" after "-e" as end-of-options (John Barboza)
    #10651
* tls:
  - new tls.TLSSocket() supports sec ctx options (Sam Roberts)
    #11005
  - Allow obvious key/passphrase combinations. (Sam Roberts)
    #10294

PR-URL: #13059
MylesBorins added a commit that referenced this pull request Jun 6, 2017
@MylesBorins
2017-06-06, Version 6.11.0 'Boron' (LTS)
4c5cbb7 
This LTS release comes with 126 commits. This includes 40 which
are test related, 32 which are doc related, 12 which are
build / tool related and 4 commits which are updates to
dependencies.

Notable Changes:

* build:
  - support for building mips64el (nanxiongchao)
    #10991
* cluster:
  - disconnect() now returns a reference to the disconnected
    worker. (Sean Villars)
    #10019
* crypto:
  - ability to select cert store at runtime (Adam Majer)
    #8334
  - Use system CAs instead of using bundled ones (Adam Majer)
    #8334
  - The `Decipher` methods `setAuthTag()` and `setAAD` now return
    `this`. (Kirill Fomichev)
    #9398
  - adding support for OPENSSL_CONF again (Sam Roberts)
    #11006
  - make LazyTransform compabile with Streams1 (Matteo Collina)
    #12380
* deps:
  - upgrade libuv to 1.11.0 (cjihrig)
    #11094
  - upgrade libuv to 1.10.2 (cjihrig)
    #10717
  - upgrade libuv to 1.10.1 (cjihrig)
    #9647
  - upgrade libuv to 1.10.0 (cjihrig)
    #9267
* dns:
  - Implemented `{ttl: true}` for `resolve4()` and `resolve6()`
    (Ben Noordhuis)
    #9296
* process:
  - add NODE_NO_WARNINGS environment variable (cjihrig)
    #10842
* readline:
  - add option to stop duplicates in history (Danny Nemer)
    #2982
* src:
  - support "--" after "-e" as end-of-options (John Barboza)
    #10651
* tls:
  - new tls.TLSSocket() supports sec ctx options (Sam Roberts)
    #11005
  - Allow obvious key/passphrase combinations. (Sam Roberts)
    #10294

PR-URL: #13059
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@mcollina @andrew749
crypto: make LazyTransform compabile with Streams1
4980c49 
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: nodejs/node#12269
PR-URL: nodejs/node#12380
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@MylesBorins @andrew749
2017-06-06, Version 6.11.0 'Boron' (LTS)
e5cdc26 
This LTS release comes with 126 commits. This includes 40 which
are test related, 32 which are doc related, 12 which are
build / tool related and 4 commits which are updates to
dependencies.

Notable Changes:

* build:
  - support for building mips64el (nanxiongchao)
    nodejs/node#10991
* cluster:
  - disconnect() now returns a reference to the disconnected
    worker. (Sean Villars)
    nodejs/node#10019
* crypto:
  - ability to select cert store at runtime (Adam Majer)
    nodejs/node#8334
  - Use system CAs instead of using bundled ones (Adam Majer)
    nodejs/node#8334
  - The `Decipher` methods `setAuthTag()` and `setAAD` now return
    `this`. (Kirill Fomichev)
    nodejs/node#9398
  - adding support for OPENSSL_CONF again (Sam Roberts)
    nodejs/node#11006
  - make LazyTransform compabile with Streams1 (Matteo Collina)
    nodejs/node#12380
* deps:
  - upgrade libuv to 1.11.0 (cjihrig)
    nodejs/node#11094
  - upgrade libuv to 1.10.2 (cjihrig)
    nodejs/node#10717
  - upgrade libuv to 1.10.1 (cjihrig)
    nodejs/node#9647
  - upgrade libuv to 1.10.0 (cjihrig)
    nodejs/node#9267
* dns:
  - Implemented `{ttl: true}` for `resolve4()` and `resolve6()`
    (Ben Noordhuis)
    nodejs/node#9296
* process:
  - add NODE_NO_WARNINGS environment variable (cjihrig)
    nodejs/node#10842
* readline:
  - add option to stop duplicates in history (Danny Nemer)
    nodejs/node#2982
* src:
  - support "--" after "-e" as end-of-options (John Barboza)
    nodejs/node#10651
* tls:
  - new tls.TLSSocket() supports sec ctx options (Sam Roberts)
    nodejs/node#11005
  - Allow obvious key/passphrase combinations. (Sam Roberts)
    nodejs/node#10294

PR-URL: nodejs/node#13059
@akaila akaila mentioned this pull request Oct 19, 2017
util: adding a unit test to cover invalid code check in util.deprecate method #16305
Closed
3 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@addaleax addaleax addaleax approved these changes
@cjihrig cjihrig cjihrig approved these changes
+1 more reviewer
@sam-github sam-github sam-github left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

crypto Issues and PRs related to the crypto subsystem. notable-change PRs with changes that should be highlighted in changelogs. stream Issues and PRs related to Node.js streams.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@mcollina @sam-github @MylesBorins @jasnell @addaleax @cjihrig @mscdex
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.