Skip to content

Conversation

@jasnell
Copy link
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 [email protected]

@nodejs-github-botnodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. labels Mar 3, 2021
@TrottTrott 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.

@aduh95aduh95force-pushed the crypto-esm-examples branch from c0f199e to 2e9ebceCompareMarch 4, 2021 22:09
@aduh95
Copy link
Contributor

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

@aduh95aduh95 removed the blocked PRs that are blocked by other issues or PRs. label Mar 4, 2021
@jasnelljasnellforce-pushed the crypto-esm-examples branch from 2e9ebce to c97c7f0CompareMarch 4, 2021 22:12
@jasnell
Copy link
MemberAuthor

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

Thank you! :-)

Copy link
Contributor

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

Signed-off-by: James M Snell <[email protected]>
@jasnelljasnellforce-pushed the crypto-esm-examples branch from c97c7f0 to 8dbdd1aCompareMarch 5, 2021 16:22
@jasnelljasnell 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
@jasnelljasnell requested a review from TrottMarch 8, 2021 14:42
Copy link
Member

@panvapanva 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
MemberAuthor

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

@benjamingrbenjamingr 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
MemberAuthor

@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
MemberAuthor

Landed in bfa6e37

@jasnelljasnell 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]>
@danielleadamsdanielleadams mentioned this pull request Mar 16, 2021
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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.cryptoIssues and PRs related to the crypto subsystem.docIssues and PRs related to the documentations.review wantedPRs that need reviews.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@jasnell@Trott@aduh95@panva@benjamingr@targos@nodejs-github-bot