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

Removing "implements Dynamic" (and implements Dynamic<T>) #6191

Closed
Simn opened this issue Apr 20, 2017 · 26 comments
Closed

Removing "implements Dynamic" (and implements Dynamic<T>) #6191

Simn opened this issue Apr 20, 2017 · 26 comments
Assignees
Milestone

Comments

@Simn
Copy link
Member

Simn commented Apr 20, 2017

Just found this in the unit tests:

#if !hl // no support for implements Dynamic yet

So it should probably be implemented. Or we take the opportunity and remove this annoying feature.

@Simn Simn added the platform-hl Everything related to HashLink label Apr 20, 2017
@Simn Simn added this to the 4.0 milestone Apr 20, 2017
@fponticelli
Copy link
Contributor

Kill it! :)

@nadako
Copy link
Member

nadako commented Apr 20, 2017

I never needed it for Haxe code, however for crazy dynamic language extern it sometimes make sense, e.g. https://github.com/nadako/hxnodejs-commander/blob/master/src/js/npm/commander/Command.hx. Maybe make it an extern-only feature? (although maybe an @:op(a.b) or something might be a better alternative).

@Justinfront
Copy link
Contributor

If it was really renamed to dynamite then it would make users feel more guilty when using, I think that it would reduce it's use, and make Simns life simpler, but Haxe is pragmatic and that's really good... but probably extremely annoying to the implementation, I don't use Dynamic but it's really important for new users - and they all stop using, it once they have properly transitioned, but to make Haxe less accessible is not productive.

@RealyUniqueName
Copy link
Member

Let's rename Dynamic to Unsafe or Hack :)

@Justinfront
Copy link
Contributor

Untyped is more accurate but Unsafe seems ideal and if you just changed it for Haxe 4 users would be prompted to check codebases.

@ncannasse
Copy link
Member

It was mostly used for Flash 8 interaction so yes I'm ok to remove it for Haxe 4 . I guess that also stands for implement Dynamic<T>

@ncannasse
Copy link
Member

And no renaming Dynamic is out of question, it would break a lot of code for no valid reason.

@nadako
Copy link
Member

nadako commented Apr 21, 2017

Yeah, Dynamic is a fine name for the type itself. I think it's similar to dynamic in C# (although it's much more used in Haxe than in C#).

This issue is about implementing Dynamic which indeed seems like a weird feature nowadays. I'm all for removing it (though we should think of some kind of replacement for externs like I linked above).

@RealyUniqueName
Copy link
Member

RealyUniqueName commented Apr 27, 2017

I +1 implements Dynamic for externs: https://github.com/HaxeFoundation/haxe/blob/development/std/php7/StdClass.hx#L5

@nadako
Copy link
Member

nadako commented Apr 27, 2017

Let's restrict it for externs for now then, and then maybe remove in favor of some good solution for externs.
This will allow simplifying some stuff in gencommon (right now it tries too hard when implementing Dynamic on classes).

If you wish I can make a PR for this.

@Justinfront
Copy link
Contributor

Justinfront commented Apr 28, 2017

Removing Dynamic will break a lot of libraries, this seems really problematic suggestion when I look at it from the perspective of developers using Haxe for real projects.

For instance git version of polygonal-ds has 181 uses in 110 *.hx files, I am sure he can work round but larger projects like OpenFL, Flambe etc... ecosystems that is going to take quite an effort Actuate is full of Dynamic.

When I search my haxelib folder through only *.hx files for the word "Dynamic".
19321 results for 14287 files.
Since it's a work computer the 60 odd libraries are all recent, half github versions of libraries covering all the toolkits, ( none of my own libraries uses any Dynamic so it's likely feasible to work without Dynamic ) but many tweening libraries rely really heavily on Dynamic.

That's about 1.3 uses of 'Dynamic' per current haxe library class perhaps more, to remove the feature will kill many haxelib projects even with fazed introduction, that's worse than just renaming it!

Loosing libraries that are used rarely, will reduce the possible soution space which already small compared to some languages.

Not sure on best approach, but I think that such a change needs to include an audit of current haxelib and larger community... or you might end up alienating some users, including comercial ones.

I am all for removing Dynamic in theory... but it in practice it could be a real worry, I would be up for banning use for new libraries but historic ones that's harder. Playing devils advocate, just airing my worry - removing Dynamic to make the compiler more robust but force significant changes in hundreds of Haxelibs, broken libraries has as much of negative effect on Haxe as obscure compiler bugs, any PR needs to address this possible problem.

@Justinfront
Copy link
Contributor

Summary 1.3 Dynamics per average haxelib class feel free to add your own local stats.

@Simn
Copy link
Member Author

Simn commented Apr 28, 2017

Guys please, this is about implements Dynamic, nothing else.

@Justinfront
Copy link
Contributor

oh sorry.

@Justinfront
Copy link
Contributor

To use a MovieClip in flash you use implements Dynamic, guess flash is less relevant now.

@RealyUniqueName
Copy link
Member

flash.display.MovieClip is an extern.

@Justinfront
Copy link
Contributor

so this would still be allowed?
public class MyAnimation extends MovieClip

@nadako
Copy link
Member

nadako commented Apr 28, 2017

Yes, but how is this related to implements Dynamic? Let's not pollute the thread with irrelevant discussions.

@Simn
Copy link
Member Author

Simn commented Sep 30, 2017

The removal of implements Dynamic is blocked on haxe.remoting being a thing. Related issue #6586.

@back2dos
Copy link
Member

Can you not make Connection an abstract and then define @:op(a.b) it, or something? Or let the generated code from Proxy/AsyncProxy just not rely on this feature (and actually generate the resolve calls)? I don't think anyone uses Connection directly, and even if they did, they can use resolve.

@Simn
Copy link
Member Author

Simn commented Sep 30, 2017

Maybe, I don't know. :) I don't really want to mess with haxe.remoting myself, in part because I have 0 test cases for it.

@back2dos
Copy link
Member

I think it's safe to say that if you remove implements Dynamic<Connection> and adjust Proxy to generate cnx.resolve("ident") instead of cnx.ident it'll be just fine.

@ncannasse ncannasse removed the platform-hl Everything related to HashLink label Feb 25, 2018
@ncannasse ncannasse removed their assignment Feb 25, 2018
@ncannasse ncannasse changed the title HL implements Dynamic is unimplemented Removing "implements Dynamic" (and implements Dynamic<T>) Feb 25, 2018
@Simn Simn modified the milestones: Release 4.0, Backlog Apr 17, 2018
@Simn Simn self-assigned this Apr 17, 2018
@Simn Simn modified the milestones: Backlog, Release 4.0 May 8, 2018
@Simn Simn modified the milestone: Release 4.0 May 8, 2018
@jgranick
Copy link

Removing this feature will break these features in OpenFL:

  • openfl.net.URLVariables

Here is an example from ActionScript:

// The first method passes the string to be decoded to the URLVariables class constructor:
var urlVariables:URLVariables = new URLVariables("firstName=Tom&lastName=Jones");
lbl.text = urlVariables.lastName + "," + urlVariables.firstName;

// The second method uses the decode() method to parse the URL encoded string:
var urlVariables:URLVariables = new URLVariables();
urlVariables.decode("firstName=Tom&lastName=Jones");
lbl.text = urlVariables.lastName + "," + urlVariables.firstName;

Workaround: Using abstract will allow the same functionality, but extending in another class or using Std.is will fail

  • openfl.display.ShaderData
var shader = new Shader ();
shader.glFragmentSource = "...";
shader.glVertexSource = "...";
shader.data.myAttribute.value = [ 1, 0, 0, 1 ];

Workaround: Using abstract or an ordinary Dynamic will allow the same functionality, but extending in another class or using Std.is will fail

  • openfl.display.MovieClip (using -Dopenfl-dynamic)
var example = Assets.getMovieClip ("library:UIDemo");
example.header.title.text = "Hello World";
trace (example.footer.width);

Workaround: There is no workaround for this behavior without breaking significant code.

cast (cast (example.getChildByName ("header"), DisplayObjectContainer).getChildByName ("title"), TextField).text = "Hello World";
trace (example.getChildByName ("footer").width);
  • openfl.errors.Error (using -Dopenfl-dynamic)

Workaround: This might not be significant. Without -D flash-strict, flash.errors.Error implements Dynamic. I am unsure how much code relies on this, though subclassing could work.

So I guess we have some work to do, but the MovieClip class is most painful

@ncannasse
Copy link
Member

@jgranick thanks for taking the time to report the usages openfl is making of it

@jgranick
Copy link

We've made the changes on our end, for some of the types, we are using the following pattern:

@:forward() abstract CustomType(Dynamic) from Dynamic to Dynamic {
    public function new () {
        this = {};
    }
}

If there are any issues with this pattern (or ways we could better emulate the previous behavior) please let me know. I understand the motivation

Gama11 referenced this issue in HaxeFoundation/hx3compat Jun 9, 2018
@Simn
Copy link
Member Author

Simn commented Sep 5, 2018

Closing this in favor of #6586 which is pretty much a superset.

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

8 participants