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

doc: crypto esm examples #37594

Closed
wants to merge 1 commit into from
Closed

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Mar 3, 2021

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 [email protected]

@nodejs-github-bot 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 Trott added the blocked PRs that are blocked by other issues or PRs. label Mar 4, 2021
@Trott
Copy link
Member

Trott commented Mar 4, 2021

Should be unblocked once #37604 lands.

@aduh95
Copy link
Contributor

aduh95 commented Mar 4, 2021

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

@aduh95 aduh95 removed the blocked PRs that are blocked by other issues or PRs. label Mar 4, 2021
@jasnell
Copy link
Member Author

jasnell commented Mar 4, 2021

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

Thank you! :-)

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

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 🤷‍♂️

doc/api/crypto.md Outdated Show resolved Hide resolved
doc/api/crypto.md Outdated Show resolved Hide resolved
Signed-off-by: James M Snell <[email protected]>
@jasnell jasnell added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. review wanted PRs that need reviews. labels Mar 5, 2021
@jasnell jasnell requested a review from Trott March 8, 2021 14:42
Copy link
Member

@panva panva left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member Author

jasnell commented Mar 9, 2021

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.

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

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

@jasnell
Copy link
Member Author

jasnell commented Mar 10, 2021

@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
Copy link
Member

panva commented Mar 10, 2021

@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
Copy link
Member Author

jasnell commented Mar 11, 2021

Landed in bfa6e37

@jasnell jasnell closed this Mar 11, 2021
jasnell added a commit that referenced this pull request Mar 11, 2021
Signed-off-by: James M Snell <[email protected]>

PR-URL: #37594
Refs: #37162
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
danielleadams pushed a commit that referenced this pull request Mar 16, 2021
Signed-off-by: James M Snell <[email protected]>

PR-URL: #37594
Refs: #37162
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
danielleadams pushed a commit that referenced this pull request Mar 16, 2021
Signed-off-by: James M Snell <[email protected]>

PR-URL: #37594
Refs: #37162
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
tniessen added a commit to tniessen/node that referenced this pull request Aug 30, 2021
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
aduh95 pushed a commit that referenced this pull request Sep 10, 2021
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 <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
BethGriggs pushed a commit that referenced this pull request Sep 21, 2021
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 <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
BethGriggs pushed a commit that referenced this pull request Sep 21, 2021
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 <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. doc Issues and PRs related to the documentations. review wanted PRs that need reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants