Skip to content
This repository has been archived by the owner on Jul 3, 2020. It is now read-only.

Add graphics subsystem #129

Closed
wants to merge 4 commits into from
Closed

Add graphics subsystem #129

wants to merge 4 commits into from

Conversation

facekapow
Copy link
Contributor

Adds a `graphics

@facekapow facekapow closed this Aug 4, 2016
@piranna
Copy link

piranna commented Aug 4, 2016

Why are you using symbols everywhere? Doesn't makes it the code more
confusing and harder to read?

El 4/8/2016 23:16, "facekapow" [email protected] escribió:

Closed #129 #129.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#129 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAgfvpIsPEjBiRbWPV0G_q5yWMa6ZU1Wks5qclapgaJpZM4JdIYV
.

@facekapow
Copy link
Contributor Author

Well this PR was accidentally submitted, it's now #130, but yeah, I could change the symbols to underscored properties, I just thought that maybe new APIs should use new features, like Promises instead of callbacks or Symbols instead of "private" underscored properties.

@piranna
Copy link

piranna commented Aug 4, 2016

For private attributes I find cleaner to use Object.defineProperty(), or to
use newer features to use a WeakMap at module level to host the class
instances private data.

El 5/8/2016 0:17, "facekapow" [email protected] escribió:

Well this PR was accidentally submitted, it's now #130
#130, but yeah, I could change
the symbols to underscored properties, I just thought that maybe new APIs
should use new features, like Promises instead of callbacks or Symbols
instead of "private" underscored properties.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#129 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAgfvoYfSJ8mBi9ZJCUvulC8RofITtTgks5qcmUJgaJpZM4JdIYV
.

@iefserge
Copy link
Member

iefserge commented Aug 5, 2016

Looks like symbols are as fast as regular properties in v8 now, nice!
I think WeakMap is going to be slower because of extra GC handling, not sure about Object.defineProperty()

@piranna
Copy link

piranna commented Aug 5, 2016

Looks like symbols are as fast as regular properties in v8 now, nice!

Good to know :-) It's just that their lexic seems to be a little bit cumbersome... :-P That's the only objection I have for them :-)

I think WeakMap is going to be slower because of extra GC handling, not sure about Object.defineProperty()

I don't know WeakMap, but code gets as clean as using private attributes and priviledged functions in the constructor without the memory waste. Object.defineProperty() I read it was slower when they were set enumerable=false, that's just what's interesting to use to define "private" attributes and functions... I try to use priviledged functions on the constructor were possible so only it's exposed the public API, and if I need to access to internal structures on subclasses or for testing I use defineProperty to define non-enumerable getters for that info. For me, clean, powerful and easy to use APIs is the top priority.

@iefserge
Copy link
Member

iefserge commented Aug 5, 2016

Yeah, performance is also important and I think Symbols are going to be the basis for private properties (if ever introduced in js). (Well, looking at the proposals, that might be wrong)

@facekapow
Copy link
Contributor Author

Well, either way, trying to have "private" properties in JavaScript is pretty much useless, anyone determined enough could get them. You can get symbols with Object.getOwnPropertySymbols(). Defining an non-enumerable simply hides it, but if someone knows it's there they can access it. I think WeakMap is the closest to truly private properties, but if you need to access them in another file (like I do with Screen), you have to export the WeakMap, and then someone can just require the file and get the WeakMap.

There is a proposal for ES7 or ES2016 (or whatever it's going to be called) for private properties (which are really more like protected properties in C++ terms), which stores all it's private properties in a private WeakMap. The catch, however, is that they are truly private, meaning the class itself is definitley the only one who can access them, plus it's only for properties, it doesn't allow hidden methods (well, maybe if you assign a function to the variable). Now what they are friends, in order to be able to access the properties from other classes if necessary (heh, I'm trying to turn JavaScript classes into C++ classes 😄).

@piranna
Copy link

piranna commented Aug 5, 2016

El 5/8/2016 14:16, "facekapow" [email protected] escribió:

I think WeakMap is the closest to truly private properties, but if you
need to access them in another file (like I do with Screen), you have to
export the WeakMap, and then someone can just require the file and get the
WeakMap.

The point is to not export the WeakMap...

There is a proposal for ES7 or ES2016 (or whatever it's going to be
called) for private properties (which are really more like protected
properties in C++ terms), which stores all it's private properties in a
private WeakMap. The catch, however, is that they are truly private,
meaning the class itself is definitley the only one who can access them,
plus it's only for properties, it doesn't allow hidden methods (well, maybe
if you assign a function to the variable). Now what they are friends, in
order to be able to access the properties from other classes if necessary
(heh, I'm trying to turn JavaScript classes into C++ classes 😄).

That's what I was asking for, a private WeakMap at the module level. You
can also have private functions by defining them at the module level and
calling them with .call() to make it orthogonal to how private attributes
are accesed on the WeakMap (the functions on the WeakMap only make sense
wheb they are on a per-instance basis.

@facekapow
Copy link
Contributor Author

Any thoughts on the API itself?

@iefserge
Copy link
Member

@facekapow #130 is the latest code? Going to comment in the PR.

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.

3 participants