FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: crypto esm examples by jasnell · Pull Request #37594 · nodejs/node · GitHub

/ node Public

doc: crypto esm examples - #37594

Closed
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:crypto-esm-examples
ConversationCommits (1)ChecksFiles changed
Closed

doc: crypto esm examples#37594
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:crypto-esm-examples

Conversation

jasnell commented Mar 3, 2021

Copy link
Copy Markdown
Member

Add ESM examples to crypto docs in preparation for #37162 landing.

Blocked for now until nodejs/remark-preset-lint-node#176 lands.

Refs: #37162

/cc @aduh95

Signed-off-by: James M Snell jasnell@gmail.com

nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. labels Mar 3, 2021
Trott added the blocked PRs that are blocked by other issues or PRs. label Mar 4, 2021

Trott commented Mar 4, 2021

Copy link
Copy Markdown
Member

Should be unblocked once #37604 lands.

aduh95 force-pushed the crypto-esm-examples branch from c0f199e to 2e9ebce Compare March 4, 2021 22:09

aduh95 commented Mar 4, 2021

Copy link
Copy Markdown
Contributor

@jasnell I've pushed updates to your branch to use mjs instead of js esm.

aduh95 removed the blocked PRs that are blocked by other issues or PRs. label Mar 4, 2021
jasnell force-pushed the crypto-esm-examples branch from 2e9ebce to c97c7f0 Compare March 4, 2021 22:12

jasnell commented Mar 4, 2021

Copy link
Copy Markdown
Member Author

I've pushed updates to your branch to use mjs instead of js esm.

Thank you! :-)

aduh95 approved these changes Mar 5, 2021
View reviewed changes

aduh95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

As with #37607, non blocking comments.

For some reason my brain sees no issue with named import for crypto (same as fs), but there's something special with assert 🤷‍♂️

Comment thread doc/api/crypto.md Outdated
Comment thread doc/api/crypto.md Outdated
doc: crypto esm examples
8dbdd1a 
Signed-off-by: James M Snell <jasnell@gmail.com>
jasnell force-pushed the crypto-esm-examples branch from c97c7f0 to 8dbdd1a Compare March 5, 2021 16:22
jasnell added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. review wanted PRs that need reviews. labels Mar 5, 2021
jasnell requested a review from Trott March 8, 2021 14:42
panva reviewed Mar 8, 2021
View reviewed changes

panva left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think the examples should use the lexical import keyword.

  • Are builds without crypto that common that all esm samples need to account for crypto missing?
  • cjs -> esm transition is ongoing and top level await import('crypto') cannot be transpiled to CJS, the samples will be a source of frustration for developers when used in their code.

jasnell commented Mar 9, 2021

Copy link
Copy Markdown
Member Author

Are builds without crypto that common that all esm samples need to account for crypto missing?

Likely not but I'd rather that folks look at this and ask why it's different than be surprised later when it doesn't work. And, to be fair, I'd like to use this as a good reason to revisit how we do this -- I'd much rather the crypto module not throw on load, and would instead just throw if any of the APIs are actually called.

benjamingr approved these changes Mar 10, 2021
View reviewed changes

benjamingr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I went over most of these examples, I only tried running a few. It would be really cool if our examples ran

jasnell commented Mar 10, 2021

Copy link
Copy Markdown
Member Author

@panva ... Would you mind if we went ahead with this PR as is (using the import() function instead of lexical import) then made the switch over to lexical once the code is changed to not throw on load?

panva commented Mar 10, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

@panva ... Would you mind if we went ahead with this PR as is (using the import() function instead of lexical import) then made the switch over to lexical once the code is changed to not throw on load?

👍

jasnell commented Mar 11, 2021

Copy link
Copy Markdown
Member Author

Landed in bfa6e37

jasnell closed this Mar 11, 2021
jasnell added a commit that referenced this pull request Mar 11, 2021
doc: crypto esm examples
bfa6e37 
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #37594
Refs: #37162
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
danielleadams pushed a commit that referenced this pull request Mar 16, 2021
doc: crypto esm examples
7530873 
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #37594
Refs: #37162
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
danielleadams mentioned this pull request Mar 16, 2021
Merged
danielleadams pushed a commit that referenced this pull request Mar 16, 2021
doc: crypto esm examples
e7fc7a4 
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #37594
Refs: #37162
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
aduh95 mentioned this pull request Jun 15, 2021
Closed
richardlau mentioned this pull request Jul 16, 2021
Merged
tniessen added a commit to tniessen/node that referenced this pull request Aug 30, 2021
doc: fix CCM cipher example in MJS
0b42990 
The original example used 'return' to terminate the current control
flow, which is valid in CommonJS. When the example was copied and
modified to use MJS syntax, the 'return' statement was left in but is
not allowed.

Refs: nodejs#37594
tniessen mentioned this pull request Aug 30, 2021
Closed
aduh95 pushed a commit that referenced this pull request Sep 10, 2021
doc: fix CCM cipher example in MJS
00ca848 
The original example used 'return' to terminate the current control
flow, which is valid in CommonJS. When the example was copied and
modified to use MJS syntax, the 'return' statement was left in but is
not allowed.

Refs: #37594

PR-URL: #39949
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
BethGriggs pushed a commit that referenced this pull request Sep 21, 2021
doc: fix CCM cipher example in MJS
4bc6f79 
The original example used 'return' to terminate the current control
flow, which is valid in CommonJS. When the example was copied and
modified to use MJS syntax, the 'return' statement was left in but is
not allowed.

Refs: #37594

PR-URL: #39949
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
BethGriggs pushed a commit that referenced this pull request Sep 21, 2021
doc: fix CCM cipher example in MJS
6d399e1 
The original example used 'return' to terminate the current control
flow, which is valid in CommonJS. When the example was copied and
modified to use MJS syntax, the 'return' statement was left in but is
not allowed.

Refs: #37594

PR-URL: #39949
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
panva mentioned this pull request Dec 16, 2022
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

panva panva approved these changes
benjamingr benjamingr approved these changes
aduh95 aduh95 approved these changes
Trott Awaiting requested review from Trott

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. review wanted PRs that need reviews.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL