hxnodejs icon indicating copy to clipboard operation
hxnodejs copied to clipboard

overriding standard types

Open nadako opened this issue 8 years ago • 45 comments

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 avatar Sep 09 '15 12:09 nadako

Bytes implementation https://github.com/HaxeFoundation/hxnodejs/commit/72d7b831dba73317f2b0e725f975ad83f8bea324

nadako avatar Sep 09 '15 12:09 nadako

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

nadako avatar Sep 09 '15 13:09 nadako

I didn't get yet whay @coreApi does :) Em 09/09/2015 10:05, "Dan Korostelev" [email protected] 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 https://github.com/HaxeFoundation/hxnodejs/issues/42#issuecomment-138903646 .

eduardo-costa avatar Sep 09 '15 14:09 eduardo-costa

@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.

nadako avatar Sep 09 '15 14:09 nadako

Got it! Thanks! We should describe more of thia in he docs! Em 09/09/2015 11:23, "Dan Korostelev" [email protected] 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 https://github.com/HaxeFoundation/hxnodejs/issues/42#issuecomment-138925704 .

eduardo-costa avatar Sep 09 '15 14:09 eduardo-costa

Yes I'm aware of the issue with Bytes redefinition. I actually opened https://github.com/HaxeFoundation/haxe/issues/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)

ncannasse avatar Sep 09 '15 21:09 ncannasse

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

waneck avatar Sep 09 '15 21:09 waneck

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 avatar Oct 13 '15 07:10 ncannasse

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 ?

ncannasse avatar Oct 29 '15 10:10 ncannasse

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.

nadako avatar Oct 29 '15 10:10 nadako

@nadako +1

clemos avatar Oct 29 '15 10:10 clemos

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.

ncannasse avatar Oct 29 '15 11:10 ncannasse

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

nadako avatar Oct 29 '15 11:10 nadako

Oh. then that makes things easy, right ?

ncannasse avatar Oct 29 '15 11:10 ncannasse

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

nadako avatar Oct 29 '15 11:10 nadako

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

ncannasse avatar Oct 29 '15 11:10 ncannasse

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

nadako avatar Oct 29 '15 11:10 nadako

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 avatar Oct 29 '15 11:10 ncannasse

(passing an UInt8Array for instance will create a copy)

ncannasse avatar Oct 29 '15 11:10 ncannasse

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

nadako avatar Oct 29 '15 18:10 nadako

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 avatar Oct 29 '15 18:10 nadako

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

nadako avatar Oct 29 '15 18:10 nadako

@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.

eduardo-costa avatar Oct 29 '15 18:10 eduardo-costa

@nadako meanwhile you can :

inline static function _byteLength ( str : String, ?enc : String ) : Int return untyped Buffer['byteLength'](str,enc);

clemos avatar Nov 03 '15 09:11 clemos

Yeah, we probably should do that for now and then deprecate it when 3.3.0 comes around

nadako avatar Nov 03 '15 10:11 nadako

I did that, pls review!

nadako avatar Nov 03 '15 13:11 nadako

Looks good to me

clemos avatar Nov 03 '15 14:11 clemos

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?

ncannasse avatar Nov 03 '15 20:11 ncannasse

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

2015-11-03 18:18 GMT-02:00 Nicolas Cannasse [email protected]:

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 https://github.com/HaxeFoundation/hxnodejs/issues/42#issuecomment-153475059 .

[image: Imagem inline 1]

eduardo-costa avatar Nov 03 '15 20:11 eduardo-costa

@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.

nadako avatar Nov 03 '15 21:11 nadako