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

Reorder example outputs #2799

Merged
merged 2 commits into from
Jun 29, 2024
Merged

Conversation

DanKaplanSES
Copy link
Contributor

Description

Remove object from examples, add 2 more examples

  • The previous example used the nullish coalescing assignment operator on const a = { duration: 50 };.
  • This new example uses the operator on primitive variables (e.g., let d = 50;).

Motivation

  • IMHO, this focuses the example on what matters. e.g., the a in a.duration ??= 10; is not required to show how ??= works, and nothing can be removed from d ??= 50;

Additional details

Relevant article: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Nullish_coalescing_assignment

Related issues and pull requests

- The previous example used the nullish coalescing assignment operator on `const a = { duration: 50 };`.
- This new example uses the operator on primitive variables (e.g., `let d = 50;`).
- IMHO, this focuses the example on what matters. e.g., the `a` in
  `a.duration ??= 10;` is not required to show how `??=` works, and
nothing can be removed from `d ??= 50;`
@DanKaplanSES DanKaplanSES requested a review from a team as a code owner June 29, 2024 21:17
@DanKaplanSES DanKaplanSES requested review from Josh-Cena and removed request for a team June 29, 2024 21:17
Copy link

It looks like this is your first pull request. 🎉 Thank you for your contribution! One of the project maintainers will triage and assign the pull request for review. We appreciate your patience. To safeguard the health of the project, please take a moment to read our code of conduct.

Copy link
Member

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

I'm not sure I like this. Interactive examples do not need to reduce the feature to a bare working minimum, because it's not a substitution for documentation. Sometimes we introduce extra semantics just to make it more realistic, and I'd say conditionally assigning properties is more realistic than assigning variables.

@DanKaplanSES
Copy link
Contributor Author

Gotcha. That's perfectly reasonable. I will say the object confused me at first until I realized it wasn't directly relevant, but I don't feel strongly about that. It's probably a me problem more than anything to do with the example.

Here's an alternative proposal: how would you feel about swapping the two code blocks in the original so it looks like this:

const a = { duration: 50 };

a.speed ??= 25;
console.log(a.speed);
// Expected output: 25

a.duration ??= 10;
console.log(a.duration);
// Expected output: 50

I think this may have avoided my confusion because:

  1. You only have to look one line up/down to see a.speed is not defined.
  2. It shows ??= performing a nullish coalesce early instead of late.

Here's a third idea:

const a = { };
a.speed ??= 25;
console.log(a.speed);
// Expected output: 25

const b = { duration: 50 };
b.duration ??= 10;
console.log(b.duration);
// Expected output: 50

Now each block of code is independent of the other; you don't have to jump over a code block to see the structure of the object you're dealing with.

FWIW, I don't feel strongly about any of these ideas--just food for thought.

@Josh-Cena
Copy link
Member

I like your first alternative, if you find it helpful too.

@DanKaplanSES
Copy link
Contributor Author

@Josh-Cena done!

@Josh-Cena Josh-Cena changed the title feat: Remove object from examples, add 2 more examples Reorder example outputs Jun 29, 2024
Copy link
Member

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

Looks good, thanks

@Josh-Cena Josh-Cena merged commit 6d27b07 into mdn:main Jun 29, 2024
5 checks passed
Copy link

Congratulations on your first merged pull request. 🎉 Thank you for your contribution! Did you know we have a project board with high-impact contribution opportunities? We look forward to your next contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants