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

overriding standard types #42

Open
nadako opened this issue Sep 9, 2015 · 45 comments
Open

overriding standard types #42

nadako opened this issue Sep 9, 2015 · 45 comments
Assignees

Comments

@nadako
Copy link
Member

nadako commented Sep 9, 2015

I tried to add haxe.io.Bytes implementation, but there's a problem that in a macro it also tries to use the overriden class and then complains about using js.node.Buffer in a macro context.

What can we do with that? One option is to just move nodejs sys stuff into Haxe itself (and fence with #if nodejs), another one is to make it possible to override types only for target output context, not the macro one.

Also, @:coreApi metadata doesn't work on our overriden types.

cc @Simn @ncannasse

@nadako
Copy link
Member Author

nadako commented Sep 9, 2015

Bytes implementation 72d7b83

@nadako
Copy link
Member Author

nadako commented Sep 9, 2015

Oh, and thanks to @andyli and Travis CI we can see that @:coreApi actually worked in 3.2.0: https://travis-ci.org/HaxeFoundation/hxnodejs/jobs/79463579

@eduardo-costa
Copy link
Contributor

I didn't get yet whay @coreapi does :)
Em 09/09/2015 10:05, "Dan Korostelev" notifications@github.com escreveu:

Oh, and thanks to @andyli https://github.com/andyli and Travis CI we
can see that @:coreApi actually worked in 3.2.0:
https://travis-ci.org/HaxeFoundation/hxnodejs/jobs/79463579


Reply to this email directly or view it on GitHub
#42 (comment)
.

@nadako
Copy link
Member Author

nadako commented Sep 9, 2015

@eduardo-costa it checks whether all fields are defined correctly in the overriden std class. it's used in haxe std for providing target implementations. it does check field existance, type, access modifiers, method signatures, etc.

@eduardo-costa
Copy link
Contributor

Got it! Thanks!
We should describe more of thia in he docs!
Em 09/09/2015 11:23, "Dan Korostelev" notifications@github.com escreveu:

@eduardo-costa https://github.com/eduardo-costa it checks whether all
fields are defined correctly in the overriden std class. it's used in haxe
std for providing target implementations. it does check field existance,
type, access modifiers, method signatures, etc.


Reply to this email directly or view it on GitHub
#42 (comment)
.

@ncannasse
Copy link
Member

Yes I'm aware of the issue with Bytes redefinition. I actually opened HaxeFoundation/haxe#4249 because of that. Not sure if that's the right solution or not.

I actually thing we need to find better ways for libraries to override/implement some std classes as well as configure which implementation is used depending on the compiled platform (in a safe manner, ie like we do for @:coreApiin stdlib)

@waneck
Copy link
Member

waneck commented Sep 9, 2015

I wish we implemented the _std magic for all classpaths - not just the std classpath

@ncannasse
Copy link
Member

This currently breaks haxe.io.BytesBuffer compilation, and breaks some WebGL code I'm having in Node Webkit as well (since I need to have Bytes that maps to JS ArrayBuffer). Should we not drop the haxe.io.Bytes <-> NodeBuffer support since it seems recent NodeJS version does uses ArrayBuffer ? (I didn't test the later)

@ncannasse
Copy link
Member

Up again : should we remove this and instead add NodeBuffer <-> Bytes conversion (in NodeBuffer class) since they are both based on ArrayBuffer but uses different APIs ?

@nadako
Copy link
Member Author

nadako commented Oct 29, 2015

To be honest, I'd remove anything but node.js externs (to reduce scope of this particular lib), and then make a separate lib that provides sys stuff.

@clemos
Copy link
Contributor

clemos commented Oct 29, 2015

@nadako +1

@ncannasse
Copy link
Member

Maybe, but that just moves the problem away, no ?
I think it would be nice for Haxe users to have a crossplatform sys API for Node. Even more if that's a complex problem to solve so that users don't have to reinvent the wheel.

But looking at the Buffer API it seems there's no way to easy convert from/to ArrayBuffer (with no copying being done in the process). I don't then understand why they say that in 4.0+, Buffer is backed by UInt8Array. Unless there's a trick, can Node users confirm this ?

If there's no easy way, I agree we should move it out of hxnodejs for now, until we come up with a solution.

@nadako
Copy link
Member Author

nadako commented Oct 29, 2015

Looking at this code: https://github.com/nodejs/node/blob/master/lib/buffer.js#L43-L62, it seems that Buffer actually IS Uint8Array

@ncannasse
Copy link
Member

Oh. then that makes things easy, right ?

@nadako
Copy link
Member Author

nadako commented Oct 29, 2015

I don't know, will have to look into it? What's the decision anyway? Support 4.0+ and provide only Sys/sys.* stuff?

@ncannasse
Copy link
Member

What I'm proposing (open for discuss):

  • support 4.0+ Buffer (which extends UInt8Array)
  • do not redefine haxe.io.Bytes and especially not BytesData (breaks several std classes and 3rd party libraries)
  • add API in Buffer (using inline methods?) which allows from/to Bytes, I can help with this
  • implement sys/Sys

@nadako
Copy link
Member Author

nadako commented Oct 29, 2015

Sounds sane, I'll try to find time to look into it today.

@ncannasse
Copy link
Member

BTW it might be possible to have the node Buffer be a reference to an ArrayBuffer (with memory sharing) if created by passing an ArrayBuffer, to confirm by experiment, see
https://github.com/nodejs/node/blob/master/lib/buffer.js#L142

@ncannasse
Copy link
Member

(passing an UInt8Array for instance will create a copy)

nadako added a commit that referenced this issue Oct 29, 2015
…+, Buffer is actually a Uint8Array on steroids.

Add helper methods to convert Buffer to/from haxe.io.Bytes. Use them for sys.io.File

See #42
@nadako
Copy link
Member Author

nadako commented Oct 29, 2015

Okay, I removed haxe.io stuff and added conversion helper methods. Please review.

@nadako
Copy link
Member Author

nadako commented Oct 29, 2015

We have a problem with Haxe 3.2.0 however: the byteLength static method of node's Buffer class has the same name as Uint8Array non-static property, so it fails to compile (https://travis-ci.org/HaxeFoundation/hxnodejs/jobs/88176317). It's allowed on Haxe development though.

@nadako
Copy link
Member Author

nadako commented Oct 29, 2015

Also, can we get rid of js.html.compat stuff for nodejs somehow?

@eduardo-costa
Copy link
Contributor

@nadako +1 to keep hxnodejs extern-only
We have the hxnodelibs repo that can fit extra libs for sys and externs for other npm tools.

@nadako
Copy link
Member Author

nadako commented Nov 3, 2015

I did that, pls review!

@clemos
Copy link
Contributor

clemos commented Nov 3, 2015

Looks good to me

@ncannasse
Copy link
Member

ok, everything seems good to me!
shall we move forward with a proper release?
or do anyone want to contribute more sys/Sys implementation (quickly) before that?

Also, we need to decide on the name. We can have the library named either "nodejs" or "hxnodejs", as soon as we have a proper -D nodejs added as well so users can #if nodejs .... In all cases, it will be the best to replace / take down no longer maintained node libraries at the same time, such as http://lib.haxe.org/p/nodejs and http://lib.haxe.org/p/nodehx if their respective authors agree with it?

@eduardo-costa
Copy link
Contributor

I'm good!
I own the nodehx lib! Feel free to put it down!

2015-11-03 18:18 GMT-02:00 Nicolas Cannasse notifications@github.com:

ok, everything seems good to me!
shall we move forward with a proper release?
or do anyone want to contribute more sys/Sys implementation (quickly)
before that?

Also, we need to decide on the name. We can have the library named either
"nodejs" or "hxnodejs", as soon as we have a proper -D nodejs added as
well so users can #if nodejs .... In all cases, it will be the best to
replace / take down no longer maintained node libraries at the same time,
such as http://lib.haxe.org/p/nodejs and http://lib.haxe.org/p/nodehx if
their respective authors agree with it?


Reply to this email directly or view it on GitHub
#42 (comment)
.

[image: Imagem inline 1]

@nadako
Copy link
Member Author

nadako commented Nov 3, 2015

@ncannasse

Also, can we get rid of js.html.compat stuff for nodejs somehow?

Because any Buffer usage right now outputs a ton of unused compat stuff in the output js.

@ncannasse
Copy link
Member

@nadako Sorry, I've let this pass. Can you not simply #if !nodejs it ?

@dionjwa
Copy link

dionjwa commented Nov 4, 2015

You can have http://lib.haxe.org/p/nodejs!

@nadako
Copy link
Member Author

nadako commented Nov 4, 2015

Okay, I added #if !nodejs guards to haxe std, seems quite clean (although it still adds empty haxe.io.Bytes constructor for some reason, but we can live with it, I guess). As for the name, I'd release it as hxnodejs and upload a deprecation release for nodejs and nodehx that point to the new lib.

@kevinresol
Copy link
Contributor

hxnodejs seems consistent with hxjava, hxcs, hxcpp, but only if it includes the sys stuff?

@nadako
Copy link
Member Author

nadako commented Nov 4, 2015

Honestly, I'm okay with any name.

@kevinresol
Copy link
Contributor

what if we name it hxnodejs and confine its scope as node extern + haxe std library?
Anything else will go to another nodelib repo

@eduardo-costa
Copy link
Contributor

We have hxnodelibs for that..
I vote for hxnodejs as extern-only
Em 04/11/2015 23:38, "Kevin Leung" notifications@github.com escreveu:

what if we name it hxnodejs and confine its scope as node extern + haxe
std library?
Anything else will go to another nodelib repo


Reply to this email directly or view it on GitHub
#42 (comment)
.

@nadako
Copy link
Member Author

nadako commented Nov 6, 2015

@kevinresol +1

If there'll be no further discussion, I can make a haxelib release.

@andyli
Copy link
Member

andyli commented Nov 6, 2015

@nadako One question: Does the JS output runs equally fine on browsers when -D nodejs? Ref. HaxeFoundation/haxe#4524

@nadako
Copy link
Member Author

nadako commented Nov 6, 2015

Oh, I gotta read that...

@nadako
Copy link
Member Author

nadako commented Nov 9, 2015

So, that topic seems to be Haxe-related which means it shouldn't block release of hxnodejs. @ncannasse waiting for you to say the word.

@nadako
Copy link
Member Author

nadako commented Nov 9, 2015

I'll close this issue, as we really should use #40 for release discussion. The Bytes problem is solved now.

@nadako
Copy link
Member Author

nadako commented Nov 9, 2015

Actually, let's leave this open for now, since the general issue is still there. Maybe it should go to Haxe's issues?

@nadako nadako reopened this Nov 9, 2015
@ncannasse
Copy link
Member

hxnodejs is ok for me. Let's release! (unless there's something else to discuss before that?)

@back2dos
Copy link
Member

@nadako The issue with the type collision can be solved by making two implementations in js.node.Bytes and js.node.BytesData and then call some --macro in extraParams.hxml to Context.defineType to alias haxe.io.Bytes and haxe.io.BytesData to them. That way the implementation will not leak into macro context.

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

No branches or pull requests

9 participants