Skip to content

Conversation

@jasnell
Copy link
Member

ESM example variants in preparation for #37162

/cc @aduh95

Signed-off-by: James M Snell [email protected]

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

I have a preference for keeping the default import for assert (assert.throws, assert.ok, assert.fail, etc.), but not blocking.

@Trott
Copy link
Member

Trott commented Mar 5, 2021

@nodejs/documentation @nodejs/assert @nodejs/modules

@Trott
Copy link
Member

Trott commented Mar 5, 2021

I hesitate to say this because I know doing this was a lot of work to begin with, but I'm with @aduh95 on the named imports. I'd prefer we do default exports because assert.throws() is a lot more clear than throws() if someone is searching through the documentation. If we want to switch to named imports/destructuring, that could be a separate PR. It would have the benefit of reducing the churn in this too.

(Full disclosure: I happen to think destructuring/named imports are a bit of an anti-pattern. If I have two modules with a get() function, it sure is a lot more clear if I'm using http.get() and https.get() at invocation rather than just get() and having to go to the require/import statement to figure out which one I'm using. So on the one hand, this is just my personal preference. But on the other hand, our example code is going to become other people's production code, so since I happen to believe that one way of doing it is actually generally better, I'd prefer we do the better thing in our example code.)

@jasnell
Copy link
MemberAuthor

What?! Lol... I'm not worried about the work involved, it's really not that much.

@aduh95aduh95 mentioned this pull request Mar 5, 2021
@jasnelljasnellforce-pushed the doc-assert-esm-examples branch from 5e11215 to 2306a36CompareMarch 5, 2021 16:23
@jasnell
Copy link
MemberAuthor

There, I fixed it for you picky folk 😁🤣

@ljharb
Copy link
Member

altho tbh the AssertionError case is the one where named imports seem clearer to me :-p fine as-is ofc!

@jasnell
Copy link
MemberAuthor

image

;-)

@jasnelljasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 5, 2021
@jasnell
Copy link
MemberAuthor

Landed in a8b5cdc

@jasnelljasnell closed this Mar 8, 2021
jasnell added a commit that referenced this pull request Mar 8, 2021
Signed-off-by: James M Snell <[email protected]> PR-URL: #37607 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Danielle Adams <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]>
danielleadams pushed a commit that referenced this pull request Mar 16, 2021
Signed-off-by: James M Snell <[email protected]> PR-URL: #37607 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Danielle Adams <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]>
@danielleadamsdanielleadams mentioned this pull request Mar 16, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

assertIssues and PRs related to the assert subsystem.author readyPRs that have at least one approval, no pending requests for changes, and a CI started.docIssues and PRs related to the documentations.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@jasnell@Trott@ljharb@benjamingr@danielleadams@aduh95@targos@nodejs-github-bot