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

[js] move standard library types to js.lib #7390

Merged
merged 7 commits into from
Apr 17, 2019

Conversation

nadako
Copy link
Member

@nadako nadako commented Sep 5, 2018

as I mentioned in #7260, it makes sense to have all "standard library" classes in js.lib sub-package so it doesn't interfere with standard types (e.g. Map) resolution from within other js packages (e.g. js.node.*). we do similar thing for python.lib, and I think it looks nicer this way.

also, as mentioned by @haxiomic in #6586 (comment), typed array classes don't really belong to js.html and probably should also be moved to js.lib (IMO not js.lib.typedarray, because js.lib conveniently represents "top-level" js standard types and typed arrays are also available as top-level)

@nadako nadako added discussion platform-javascript Everything related to JS / JavaScript labels Sep 5, 2018
@nadako
Copy link
Member Author

nadako commented Sep 5, 2018

@Simn by the way, do you think we could auto-generate a nicer deprecation message for @:deprecated typedef Name = TypePath case, so it adds "type was moved into TypePath" or something automatically?

@Simn
Copy link
Member

Simn commented Sep 5, 2018

How exactly would that be better? To the user it only matters what he should use instead.

@nadako
Copy link
Member Author

nadako commented Sep 5, 2018

How exactly would that be better? To the user it only matters what he should use instead.

At the moment it outputs the default Warning : Usage of this typedef is deprecated, not pointing the user to the type they should use instead, so I have to manually pass the warning message. I'm just wondering if it makes sense to generate one if we do @:deprecated typedef Name = some.Type.Path so it emits Warning : Usage of this typedef is deprecated. Use some.Type.Path instead.

@Simn
Copy link
Member

Simn commented Sep 5, 2018

Ah, I thought it said something like that. In that case I agree we can maybe improve the message.

@ncannasse
Copy link
Member

ncannasse commented Sep 6, 2018

That seems like a lot of breaking changes. I would prefer we rename js.Map to js.NativeMap instead, that's what we do with other platforms as well.

@skial skial mentioned this pull request Sep 6, 2018
1 task
@nadako
Copy link
Member Author

nadako commented Sep 6, 2018

That seems like a lot of breaking changes.

All of the types except Error, RegExp and Promise were added in Haxe 4, and we still have backward-compatibility typedefs, so I'm not sure about "breaking".

I would prefer we rename js.Map to js.NativeMap instead, that's what we do with other platforms as well.

On some we do, on others we don't. I personally find the approach with separate package and the same names as native cleaner, because they are easier to find, and also I don't like mixing "support" classes like js.Syntax with standard lib, because of possible name clashes (e.g. ecmascript adds Syntax class and we end up with Syntax and NativeSyntax, meh).

But anyway, I don't have that strong position on this, maybe some js target power users want to comment here? (cc @elsassph @back2dos)

@haxiomic
Copy link
Member

haxiomic commented Sep 9, 2018

On TypedArrays: it's safe to move them out of js.html – although they're autogenerated there's not much value in generating them the from handwritten WebIDLs which is the case at the moment (there isn't actually any official WebIDLs for TypedArrays).

Then we just need to add typedefs to html-externs/haxe/js/html so that they don't get lost when externs are generated

@elsassph
Copy link
Contributor

Not a fan of Native, and a bit concerned about the package change: does it affect js.Promise for instance?
TypedArray though shouldn't be under js.html if they aren't browser specific.

@back2dos
Copy link
Member

Not sure what "lib" is supposed to mean. Seems like "global" would be a better choice of name (it's how JavaScript people would call it). Perhaps "native" would work too (it's how Haxe people would call it).

Other than that I pretty much like these changes. I too prefer to keep the names close to what the native stuff is, as that makes porting easier.

@ncannasse
Copy link
Member

Breaking js.Promise seems like a bad idea.

@nadako
Copy link
Member Author

nadako commented Sep 12, 2018

It's not breaking, there are backward-compatibility typedefs.

@ncannasse
Copy link
Member

with a deprecated warning, which might trigger hundreds of warnings in code using it.

@back2dos
Copy link
Member

Breaking js.Promise seems like a bad idea.
[...]
with a deprecated warning, which might trigger hundreds of warnings in code using it.

A quick github search:

  1. js.Promise - 450 results
  2. Lambda - 8,395 results

There's a few false positives in the latter result set, but I think it's still blatantly clear that we have far more serious breaking changes, that by the way don't result in a warning, but an error.

So let's be consistent.

Personally, I think the best solution is to put the typedefs into hx3compat and add the deprecation warnings there behind a flag to turn them on. This will provide a single gradual migration strategy that can be communicated clearly.

@nadako
Copy link
Member Author

nadako commented Sep 13, 2018

Personally, I think the best solution is to put the typedefs into hx3compat and add the deprecation warnings there behind a flag to turn them on. This will provide a single gradual migration strategy that can be communicated clearly.

This sounds good to me, I totally forgot about hx3compat!

@elsassph
Copy link
Contributor

hx3compat idea is good for projects - libraries will have a harder time but that's manageable.

@Simn
Copy link
Member

Simn commented Dec 12, 2018

What's the status here?

@ncannasse
Copy link
Member

I'm still against changing that. Renaming js.Map to js.NativeMap should solve the original problem.

@back2dos
Copy link
Member

Having Object and Symbol vs. NativeDate and NativeMap is inconsistent. Either we should prefix all of them, or none. I prefer the latter, because then you can just go import js.global.*; (or std or native) and port JS code with much less friction.

@haxiomic
Copy link
Member

haxiomic commented Feb 12, 2019

I'm conflicted because the Native* prefix seems to be the most common pattern with the other targets but I think we set ourselves up for a repeat of these issues if we don't make it a strict convention for all top-level js types since it's possible we will have other conflicts in the future when new types are added to the haxe top-level or js top-level (like Error, Promise, Reflect) and we then have to do the rename later down the line. When we have a prefix used uniformly for each type like that, it becomes cleaner to move them into something like js.native

Additionally, the js typedarrays need moving out of html into the js top-level, so we can end up with NativeFloat32Array, and it's quite a nice to be able to swap the implementation by changing the imports like @back2dos mentioned

@nadako
Copy link
Member Author

nadako commented Feb 12, 2019

I'm still against Native* approach. I think it's confusing, misleading and not future-proof.

@kevinresol
Copy link
Contributor

I am fine with any name as long as they are moved. Because js.Date has been breaking hxnodejs for a long time. Given typed arrays has moved into js.lib I think we can just follow that.

@Simn Simn added this to the Release 4.0 milestone Mar 21, 2019
@Simn
Copy link
Member

Simn commented Mar 21, 2019

Agreed, we should do this for 4.0 final.

@RealyUniqueName
Copy link
Member

Conflicts have been resolved.
So, what about js.Date and js.Map? Should they be removed completely instead of just deprecated?

@kevinresol
Copy link
Contributor

kevinresol commented Apr 16, 2019

So, what about js.Date and js.Map? Should they be removed completely instead of just deprecated?

Please remove, it breaks all libraries that reside in the js package (e.g. hxnodejs)

@RealyUniqueName
Copy link
Member

I'd remove them, but what about @ncannasse ? :)

@ncannasse
Copy link
Member

Good for me with removal

@RealyUniqueName
Copy link
Member

CI failure is not related.
Pushing the removal and merging...

@RealyUniqueName RealyUniqueName merged commit 88e5195 into HaxeFoundation:development Apr 17, 2019
Gama11 pushed a commit to HaxeFoundation/hxnodejs that referenced this pull request Apr 17, 2019
Gama11 added a commit to vshaxe/haxe-TmLanguage that referenced this pull request Apr 17, 2019
@Gama11 Gama11 changed the title [js] move standard library types to js.lib (DON'T MERGE YET, let's discuss) [js] move standard library types to js.lib Apr 17, 2019
Gama11 added a commit to vshaxe/haxe-language-server that referenced this pull request Apr 17, 2019
RealyUniqueName pushed a commit to RealyUniqueName/JStack that referenced this pull request Apr 17, 2019
Gama11 added a commit to vshaxe/vscode-extern that referenced this pull request Apr 17, 2019
Gama11 added a commit to vshaxe/vshaxe that referenced this pull request Apr 17, 2019
Gama11 added a commit to vshaxe/vshaxe-debug-tools that referenced this pull request Apr 17, 2019
Gama11 added a commit to vshaxe/vscode-debugadapter-extern that referenced this pull request Apr 17, 2019
Gama11 added a commit to vshaxe/flash-debugger that referenced this pull request Apr 17, 2019
Gama11 added a commit to vshaxe/codedox that referenced this pull request Apr 17, 2019
Gama11 added a commit to vshaxe/haxe-test-adapter that referenced this pull request Apr 17, 2019
Gama11 added a commit that referenced this pull request Apr 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion platform-javascript Everything related to JS / JavaScript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants