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

Updated and added pages for private class features #5690

Merged
merged 11 commits into from Jun 22, 2021
Merged

Conversation

meyerweb
Copy link
Contributor

@meyerweb meyerweb commented Jun 4, 2021

Private class features needed some better explanation on their reference page, and a short-ish guide to their use added. I developed these changes and additions in consultation with folks at Igalia who did the private class feature implementation in Blink.

@meyerweb meyerweb requested a review from a team as a code owner June 4, 2021 19:41
@meyerweb meyerweb requested review from sideshowbarker and removed request for a team June 4, 2021 19:41
@github-actions
Copy link
Contributor

github-actions bot commented Jun 4, 2021

Preview URLs

Flaws

Note! 3 documents with no flaws that don't need to be listed. 🎉

URL: /en-US/docs/Web/JavaScript/Guide/Working_With_Private_Class_Features
Title: Working with private class features
on GitHub
Flaw count: 2

  • bad_bcd_links:
    • no explanation!
    • no explanation!

External URLs

URL: /en-US/docs/Web/JavaScript/Guide/Working_With_Private_Class_Features
Title: Working with private class features
on GitHub


URL: /en-US/docs/Web/JavaScript/Reference/Classes
Title: Classes
on GitHub


URL: /en-US/docs/Web/JavaScript/Reference/Classes/Private_class_fields
Title: Private class features
on GitHub


URL: /en-US/docs/Web/JavaScript/Reference/Classes/Public_class_fields
Title: Public class fields
on GitHub

(this comment was updated 2021-06-21 18:42:08.099282)

- JavaScript
- Language feature
browser-compat: javascript.classes.private_class_fields
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned in the meeting, this might have been a bug in the yari tooling or a matter of a merge conflict of old files.
We should keep this line and then revert back to <p>{{Compat}}</p> somewhere around line 250.

Comment on lines 249 to 261
<table class="standard-table">
<thead>
<tr>
<th scope="col">Specification</th>
</tr>
</thead>
<tbody>
<tr>
<td>{{SpecName('Public and private instance fields', '#prod-FieldDefinition',
'FieldDefinition')}}</td>
</tr>
</tbody>
</table>
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, these lines should reverted to just {{Specifications}}.


<h2 id="Browser_compatibility">Browser compatibility</h2>

<p>{{Compat}}</p>
<p>{{Compat("javascript.classes.public_class_fields")}}</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto :)

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Thanks Eric, this looks great. I had some comments, mostly copy-editing things.

It would be worth drafting a consistent note describing the fact that this is a proposal and not part of the main standard (as I understand it) and using it across all the pages here.

<p>Private class features deliver truly private fields and methods, with that privacy enforced by the language instead of convention. This confers benefits such as avoiding naming collisions between class features and the rest of the code base, and allowing classes to expose a very small interface to the rest of the code.</p>


<h2>Private Fields</h2>
Copy link
Collaborator

Choose a reason for hiding this comment

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

We use sentence case, so this should probably be "Private fields".


<p>To understand how private fields work, let’s first consider a class that has only public fields, but uses the constructor to encapsulate data—a somewhat common technique, even if it is a bit of a hack. The following class creates a basic count that accepts a starting number, allows that number to be increased or decreased, and can be reset to the original starting value or any other value.</p>

<pre class="brush: js example-bad">
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm very much on the fence about this, but giving this example "bad-example" styling feels a little harsh. It's not actually broken after all, it's just not as good as the new thing.

}
</pre>

<p>And you can have static private fields, for things you want to be both private and set in stone at construction.</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's weird to have this bit about a thing you can do in the middle of a bit about things you can't do.
Why not an H3 "Static private fields" with this bit, followed by an H3 of "Limitations of private fields" with the rest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried addressing this without adding subheadings in the latest push; let me know if it feels like it works. If not, I’ll see about using headings.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I like this approach :).


<h2 id="See_also">See also</h2>

<ul>
<li><a href="/en-US/docs/Web/JavaScript/Reference/Classes/Public_class_fields">Public
class fields</a></li>
<li><a href="https://v8.dev/features/class-fields">Public and private class fields</a> article at the v8.dev site.</li>
<li><a href="https://github.com/tc39/proposal-class-fields#class-field-declarations-for-javascript">Class field declarations for JavaScript</a> explainer, by the <a href="https://github.com/tc39/proposal-class-fields">Public and private instance fields</a> authors</li>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Link to the new guide page?

@@ -5,14 +5,13 @@
- Classes
- JavaScript
- Language feature
browser-compat: javascript.classes.public_class_fields
Copy link
Collaborator

Choose a reason for hiding this comment

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

As Peter said above, we should have this in front matter and remove it from the {{Compat}} call.

---
<div>{{JsSidebar("Classes")}}</div>

<div class="note">
<p><strong>Note:</strong> This page describes experimental features.</p>
<p><strong>This page describes experimental features.</strong></p>
Copy link
Collaborator

@wbamberg wbamberg Jun 7, 2021

Choose a reason for hiding this comment

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

As noted before we might want to consider using a common form of wording across all three pages for this issue. Perhaps rather than loaded words like "experimental" we could say that it is a proposal and not yet part of the ES spec? Also the formatting of the note has be be pedantically exact for the Markdown conversion to work properly. It has to be <strong>Note:</strong>.


<p>Both public and private field declarations are an <a
<p>Both Public and private field declarations are an <a
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure why this change was made.

@@ -275,17 +274,27 @@ <h4 id="Public_instance_methods">Public instance methods</h4>

<h2 id="Specifications">Specifications</h2>

{{Specifications}}
<table class="standard-table">
Copy link
Collaborator

Choose a reason for hiding this comment

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

As before, we should revert this change and just have {{Specifications}} here, then it will pick up the spec link from the BCD front matter item.

@meyerweb
Copy link
Contributor Author

meyerweb commented Jun 8, 2021

Based on the above conversation(s), I’m going to choose to keep to a single page rather splitting into two pages. Given that, I’ll address the requests and update. Thanks, all!

@meyerweb
Copy link
Contributor Author

meyerweb commented Jun 8, 2021

I made edits and have pushed changes. I think I addressed all the requested changes, but the way GitHub mixes requests up so that files aren’t grouped together made it hard to be sure. Let me know if anything else is needed.

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! It looks like the reinstated browser-compat front matter item for public fields needs to be javascript.classes.public_class_fields, not javascript.classes.private_class_fields?

There were a couple of other comments that still might need attention (but please do let me know if you don't think they are applicable):

Finally I wonder if it's worth having some common way to describe the status of these features.

It might be good to use a common message across all three pages - perhaps a modified version of the note in https://pr5690.content.dev.mdn.mozit.cloud/en-US/docs/Web/JavaScript/Reference/Classes/Public_class_fields ?

@@ -5,7 +5,7 @@
- Classes
- JavaScript
- Language feature
browser-compat: javascript.classes.public_class_fields
browser-compat: javascript.classes.private_class_fields
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be browser-compat: javascript.classes.public_class_fields ?

Copy link
Contributor Author

@meyerweb meyerweb Jun 9, 2021

Choose a reason for hiding this comment

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

No, it should be javascript.classes.private_class_fields. They’re different things. I beg your pardon, I misspoke. That was misread and an error on my part, and I’ll fix it. Sorry about that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Hit return too soon) As for the two-way links, agreed, and I missed those. I’ll harmonize the warning notes as well. Stand by!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sideshowbarker makes a good point: the warnings are about to become moot, and should be removed. So I’ll harmonize by removing the warnings and patching the holes. Once more, stand by!

@sideshowbarker
Copy link
Collaborator

Finally I wonder if it's worth having some common way to describe the status of these features.

* [pr5690.content.dev.mdn.mozit.cloud/en-US/docs/Web/JavaScript/Reference/Classes/Public_class_fields](https://pr5690.content.dev.mdn.mozit.cloud/en-US/docs/Web/JavaScript/Reference/Classes/Public_class_fields) has a note saying that public class fields are an experimental stage 3 TC39 proposal.

* [pr5690.content.dev.mdn.mozit.cloud/en-US/docs/Web/JavaScript/Reference/Classes/Private_class_fields](https://pr5690.content.dev.mdn.mozit.cloud/en-US/docs/Web/JavaScript/Reference/Classes/Private_class_fields) says private fields are stage 3 but not as a note and not mentioning experimental.

It seems both of those have outdated status info at this point, and should be removed. See c59c122 and #5850 — the Class Fields proposal was merged into the ES spec in April. And because also in April, Safari 14.1 shipped with full support for class fields, that means all browser engines now have full support.

class. There is however <a href="https://github.com/tc39/proposal-private-methods">a
stage 3 proposal</a> to allow defining private class fields using a hash
<code>#</code> prefix.</p>
stage 3 proposal</a> to allow private class features by using a hash
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that the proposal was merged into the ES spec April, the part part about There is however a stage 3 proposal to allow private class features by using a hash # prefix should be changed to just, However, private class fields can be created using a hash # prefix, or some such.

And also, I think everywhere the content says class properties, it should instead say class fields.

@hamishwillee
Copy link
Collaborator

What's blocking this? Anything I can do to help?

@meyerweb
Copy link
Contributor Author

What's blocking this? Anything I can do to help?

Actually, I’m not sure if this just needs review, or if it needs more updates. @wbamberg, I think you’re the arbiter of that, yes?

@wbamberg
Copy link
Collaborator

Most of the comments in #5690 (review) are addressed (thanks!). The only remaining thing in there is to update the various notes and things about it being experimental, not in the ES spec and so on.

@sideshowbarker already removed the note about this from "public class fields" (c59c122), so the only remaining part is the suggestion in #5690 (comment) to drop the reference to stage 3 in "private class fields". With that change I'm 👍 on this PR.

@sideshowbarker also had this comment in #5690 (comment) :

I think everywhere the content says class properties, it should instead say class fields.

@meyerweb
Copy link
Contributor Author

I think I’ve resolved the lingering issues, but let me know if I missed any.

@hamishwillee
Copy link
Collaborator

@wbamberg Anything else you need from this?

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Thanks for all your work on this, Eric!

@wbamberg wbamberg merged commit 8251599 into mdn:main Jun 22, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants