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

stream: do not use crypto.DEFAULT_ENCODING in lazy_transform.js #24396

Closed
wants to merge 1 commit into from
Closed

stream: do not use crypto.DEFAULT_ENCODING in lazy_transform.js #24396

wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

The default encoding can be retrieved via
require('internal/crypto/util').getDefaultEncoding instead of
the deprecated crypto.DEFAULT_ENCODING which triggers a warning.

Background:

The require chain goes like this:

internal/streams/lazy_transform.js
  -> crypto.js
  -> internal/crypto/cipher.js (uses LazyTransform in the global scope)
  -> internal/streams/lazy_transform.js

So when internal/streams/lazy_transform.js is required before
lib/crypto.js, we have a circular dependency and since
internal/crypto/cipher.js uses destructuring to use LazyTransform
we will get an error. And it can also trigger a warning if
lazy_transform.js is the first file that touches
crypto.DEFAULT_ENCODING.

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

@joyeecheung
stream: do not use crypto.DEFAULT_ENCODING in lazy_transform.js
152a8f6 
The default encoding can be retrieved via
`require('internal/crypto/util').getDefaultEncoding` instead of
the deprecated crypto.DEFAULT_ENCODING which triggers a warning.

Background:

The require chain goes like this:

```
internal/streams/lazy_transform.js
  -> crypto.js
  -> internal/crypto/cipher.js (uses LazyTransform in the global scope)
  -> internal/streams/lazy_transform.js
```

So when `internal/streams/lazy_transform.js` is required before
`lib/crypto.js`, we have a circular dependency and since
`internal/crypto/cipher.js` uses destructuring to use LazyTransform
we will get an error. And it can also trigger a warning if
lazy_transform.js is the first file that touches
crypto.DEFAULT_ENCODING.
@joyeecheung joyeecheung requested a review from jasnell November 16, 2018 16:38
@joyeecheung joyeecheung mentioned this pull request Nov 16, 2018
Closed
4 tasks
@joyeecheung
Copy link
Member Author

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

This unblocks #24384 see #24384 (comment)

@joyeecheung
Copy link
Member Author

On a side note, in general we should strive to avoid requiring non-internal modules in internal modules so that we can load fewer unnecessary modules, because non-internal modules then to have a bigger dependency graph so once you require one of them it could turn into a dominoes game

@refack refack added the crypto Issues and PRs related to the crypto subsystem. label Nov 16, 2018
@joyeecheung joyeecheung added the stream Issues and PRs related to the stream subsystem. label Nov 16, 2018
@refack refack added lib / src Issues and PRs related to general changes in the lib or src directory. fast-track PRs that do not need to wait for 48 hours to land. and removed stream Issues and PRs related to the stream subsystem. fast-track PRs that do not need to wait for 48 hours to land. labels Nov 16, 2018
@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 16, 2018
@joyeecheung joyeecheung mentioned this pull request Nov 16, 2018
Closed
4 tasks
@Trott
Copy link
Member

Trott commented Nov 18, 2018

Landed in 413fcad

@Trott Trott closed this Nov 18, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 18, 2018
@joyeecheung @Trott
stream: do not use crypto.DEFAULT_ENCODING in lazy_transform.js
413fcad 
The default encoding can be retrieved via
`require('internal/crypto/util').getDefaultEncoding` instead of
the deprecated crypto.DEFAULT_ENCODING which triggers a warning.

Background:

The require chain goes like this:

```
internal/streams/lazy_transform.js
  -> crypto.js
  -> internal/crypto/cipher.js (uses LazyTransform in the global scope)
  -> internal/streams/lazy_transform.js
```

So when `internal/streams/lazy_transform.js` is required before
`lib/crypto.js`, we have a circular dependency and since
`internal/crypto/cipher.js` uses destructuring to use LazyTransform
we will get an error. And it can also trigger a warning if
lazy_transform.js is the first file that touches
crypto.DEFAULT_ENCODING.

PR-URL: nodejs#24396
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request Nov 19, 2018
@joyeecheung @targos
stream: do not use crypto.DEFAULT_ENCODING in lazy_transform.js
529744e 
The default encoding can be retrieved via
`require('internal/crypto/util').getDefaultEncoding` instead of
the deprecated crypto.DEFAULT_ENCODING which triggers a warning.

Background:

The require chain goes like this:

```
internal/streams/lazy_transform.js
  -> crypto.js
  -> internal/crypto/cipher.js (uses LazyTransform in the global scope)
  -> internal/streams/lazy_transform.js
```

So when `internal/streams/lazy_transform.js` is required before
`lib/crypto.js`, we have a circular dependency and since
`internal/crypto/cipher.js` uses destructuring to use LazyTransform
we will get an error. And it can also trigger a warning if
lazy_transform.js is the first file that touches
crypto.DEFAULT_ENCODING.

PR-URL: #24396
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@joyeecheung @rvagg
stream: do not use crypto.DEFAULT_ENCODING in lazy_transform.js
7bc2011 
The default encoding can be retrieved via
`require('internal/crypto/util').getDefaultEncoding` instead of
the deprecated crypto.DEFAULT_ENCODING which triggers a warning.

Background:

The require chain goes like this:

```
internal/streams/lazy_transform.js
  -> crypto.js
  -> internal/crypto/cipher.js (uses LazyTransform in the global scope)
  -> internal/streams/lazy_transform.js
```

So when `internal/streams/lazy_transform.js` is required before
`lib/crypto.js`, we have a circular dependency and since
`internal/crypto/cipher.js` uses destructuring to use LazyTransform
we will get an error. And it can also trigger a warning if
lazy_transform.js is the first file that touches
crypto.DEFAULT_ENCODING.

PR-URL: #24396
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
Merged
4 tasks
This was referenced Dec 7, 2018
Closed
Closed
Closed
Closed
Closed
codebytere pushed a commit that referenced this pull request Jan 13, 2019
@joyeecheung @codebytere
stream: do not use crypto.DEFAULT_ENCODING in lazy_transform.js
33318cd 
The default encoding can be retrieved via
`require('internal/crypto/util').getDefaultEncoding` instead of
the deprecated crypto.DEFAULT_ENCODING which triggers a warning.

Background:

The require chain goes like this:

```
internal/streams/lazy_transform.js
  -> crypto.js
  -> internal/crypto/cipher.js (uses LazyTransform in the global scope)
  -> internal/streams/lazy_transform.js
```

So when `internal/streams/lazy_transform.js` is required before
`lib/crypto.js`, we have a circular dependency and since
`internal/crypto/cipher.js` uses destructuring to use LazyTransform
we will get an error. And it can also trigger a warning if
lazy_transform.js is the first file that touches
crypto.DEFAULT_ENCODING.

PR-URL: #24396
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@joyeecheung @refack
stream: do not use crypto.DEFAULT_ENCODING in lazy_transform.js
17e6c59 
The default encoding can be retrieved via
`require('internal/crypto/util').getDefaultEncoding` instead of
the deprecated crypto.DEFAULT_ENCODING which triggers a warning.

Background:

The require chain goes like this:

```
internal/streams/lazy_transform.js
  -> crypto.js
  -> internal/crypto/cipher.js (uses LazyTransform in the global scope)
  -> internal/streams/lazy_transform.js
```

So when `internal/streams/lazy_transform.js` is required before
`lib/crypto.js`, we have a circular dependency and since
`internal/crypto/cipher.js` uses destructuring to use LazyTransform
we will get an error. And it can also trigger a warning if
lazy_transform.js is the first file that touches
crypto.DEFAULT_ENCODING.

PR-URL: nodejs#24396
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@codebytere codebytere mentioned this pull request Jan 15, 2019
Merged
codebytere pushed a commit that referenced this pull request Jan 29, 2019
@joyeecheung @codebytere
stream: do not use crypto.DEFAULT_ENCODING in lazy_transform.js
a7c1d09 
The default encoding can be retrieved via
`require('internal/crypto/util').getDefaultEncoding` instead of
the deprecated crypto.DEFAULT_ENCODING which triggers a warning.

Background:

The require chain goes like this:

```
internal/streams/lazy_transform.js
  -> crypto.js
  -> internal/crypto/cipher.js (uses LazyTransform in the global scope)
  -> internal/streams/lazy_transform.js
```

So when `internal/streams/lazy_transform.js` is required before
`lib/crypto.js`, we have a circular dependency and since
`internal/crypto/cipher.js` uses destructuring to use LazyTransform
we will get an error. And it can also trigger a warning if
lazy_transform.js is the first file that touches
crypto.DEFAULT_ENCODING.

PR-URL: #24396
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This was referenced Jan 29, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@addaleax addaleax addaleax approved these changes

@refack refack refack approved these changes

@sam-github sam-github sam-github approved these changes

@jasnell jasnell Awaiting requested review from jasnell

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. crypto Issues and PRs related to the crypto subsystem. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@joyeecheung @Trott @sam-github @refack @addaleax @TimothyGu @cjihrig