search

cubing / cubing.js

🛠 A library for displaying and working with twisty puzzles. Also currently home to the code for Twizzle.
https://js.cubing.net/cubing/
GNU General Public License v3.0
231 stars 42 forks source link

Change all private methods on ManagedCustomElement subclasses to #private #114

Open lgarron opened 3 years ago

lgarron commented 3 years ago

I did this for TwistyPlayer here: ad42e3d20913cabebdfa08c4146b48a3ee55e2d8

I'd like to do the same for public objects, especially these classes: https://github.com/cubing/cubing.js/search?q=extends+ManagedCustomElement

V-Wong commented 3 years ago

Hi, I'm interested on working on this issue.

Just to clarify, do you want to convert just the methods, or both the methods and properties?

rokicki commented 3 years ago

I'm a tiny bit concerned about this from at least two perspectives:

  1. Testing. Until our test coverage is even reasonable, what will we be silently breaking? Should we prefer increasing our test coverage first?

  2. Performance. V8 does a lot of work to make properly access reasonably fast, and using the new #xxx private support introduces a WeakMap indirection that could potentially seriously affect overall performance. While many of our classes are not performance critiical we could easily die the death of a thousand cuts.

This is all kind of unfortunate; C++/Java history with access control modifiers give the impression that access control is absolutely free, but JavaScript/Typescript's shoehorning of the concept onto the existing language imposes a difficult to predict and difficult to measure performance cost. I'd be tempted to hold off and see what happens in this space before going through and adding # privacy decorators all over our code.

But then, I'm probably the least experienced Typescript programmer here.

On Mon, Jun 7, 2021 at 2:33 AM Vincent Wong @.***> wrote:

Hi, I'm interested on working on this issue.

Just to clarify, do you want to convert just the methods, or both the methods and properties?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/cubing/cubing.js/issues/114#issuecomment-855771860, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMOLSZU3QB5YTYQJJGY623TRSG5XANCNFSM45VP6S4Q .

--

V-Wong commented 3 years ago

Definitely agree with that first point there. Are there any particular areas that I could help with in terms of test coverage?

rokicki commented 3 years ago

Oh yeah definitely! The "make test" runs our existing tests and also gives concise coverage information. I'd pick one of the modules/files that shows up in red and see if you can add a test or two that increases coverage (while, of course, also attempting to test that the functionality is as expected).

Don't be shy about pushing your first changes; things will go much easier if you make changes that we can see and review rather than spinning too long on any particular thing.

Thanks!

-tom

On Mon, Jun 7, 2021 at 9:43 AM Vincent Wong @.***> wrote:

Definitely agree with that first point there. Are there any particular areas that I could help with in terms of test coverage?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/cubing/cubing.js/issues/114#issuecomment-856094750, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMOLSZ5NVEUW7GVIEVJRUTTRTZK7ANCNFSM45VP6S4Q .

--

jxu commented 3 years ago

Is this a good idea for compatibility? AFAIK js private fields are still a proposal and not supported in firefox.

lgarron commented 3 years ago

Hi, I'm interested on working on this issue.

Just to clarify, do you want to convert just the methods, or both the methods and properties?

Glad to hear you're interested!

Both are actually valuable:

With a sufficient number of users of an API, it does not matter what you promise in the contract: all observable behaviors of your system will be depended on by somebody. https://www.hyrumslaw.com/

In any case, there's not a clear line between these now that JS has getters and setters.

The best way to make sure that no one mistakes our private fields and method for APIs is to make them inaccessible, at least by default. In order of usefulness, I think it would help to:

This could also go hand-in-hand with organizing the order of methods for those classes, and adding documentation.

  1. Testing. Until our test coverage is even reasonable, what will we be silently breaking? Should we prefer increasing our test coverage first?

I definitely agree that testing and documentation are more important priorities.

That said, it would be useful to take steps because the documentation should exactly match the public API in the end. I'm not so worried about the risk of breakage, because a good editor should be able to do the renames easily, and TypeScript would catch a lot of simple hand errors.

Also, if we ever want to expose private methods for testing only, that's not too difficult. And we can easily change a #private method to something more public. So we're not really making our life much harder for the future.

  1. Performance. V8 does a lot of work to make properly access reasonably fast, and using the new #xxx private support introduces a WeakMap indirection that could potentially seriously affect overall performance. While many of our classes are not performance critiical we could easily die the death of a thousand cuts.

I don't know so much about this one.

I think it's more important to start with the right API, but there are a few places private fields/methods are in the hot path (like Twisty3DCanvas) and this would create more. That's why I made this issue specifically about ManagedCustomElement classes. It would be nice to get more data before we do this, say, all over KPuzzle, though.

Is this a good idea for compatibility? AFAIK js private fields are still a proposal and not supported in firefox.

We transpile our builds, so this is not an issue. Private methods and fields work similar to functions defined in a closure, which are a very old concept. (See line 3800 of https://unpkg.com/browse/cubing@0.18.2/dist/esm/chunk-UOUXNU34.js for example.)

lgarron commented 3 years ago

Private methods and fields work similar to functions defined in a closure, which are a very old concept.

@rokicki just pointed out to me that these are actually implemented in our builds using WeakMap, which I totally missed. I'll have to look into it a bit more. 😳

If, hypothetically, private fields/methods were suuuuper unperformant, I would advocate we switch to an obvious prefix for non-API fields/methods, such as experimental (which we're already using in a few places), private, or _.

lgarron commented 2 years ago

The latest Firefox Nightly and Safari Technology Preview include #private methods, which means this will be available in all modern browsers soon. This means 1) we can test perf in all those, and 2) it's moderately safe to start using this in more places without having to rely on WeakMap past the end of the year.