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

feat(node): v11.6 #32878

Merged
merged 3 commits into from
Feb 12, 2019
Merged

feat(node): v11.6 #32878

merged 3 commits into from
Feb 12, 2019

Conversation

SimonSchick
Copy link
Contributor

@SimonSchick SimonSchick commented Feb 7, 2019

Please fill in this template.

If changing an existing definition:

This adds misc things from node v11.6, some of the changes are being worked on by others so I didn't add those, see:

In addition I've started moving module tests into the /tests folder to avoid constantly having to touch this overly large file.

#32787
#32835
#32597

@rbuckton as a note, would you be fine with removing typings for all node version that are EOL? (v0, v4, v5, v7, v9) it should prevent people from editing old/obsolete typings.

Also:
fixes #32744
fixes #32926
Partially addresses #31505

@typescript-bot typescript-bot added Where is Travis? New Definition This PR creates a new definition package. Popular package This PR affects a popular package (as counted by NPM download counts). Author is Owner The author of this PR is a listed owner of the package. labels Feb 7, 2019
@typescript-bot
Copy link
Contributor

typescript-bot commented Feb 7, 2019

@SimonSchick Thank you for submitting this PR!

Because this is a new definition, a DefinitelyTyped maintainer will be reviewing this PR in the next few days once the Travis CI build passes.

In the meantime, if the build fails or a merge conflict occurs, I'll let you know. Have a nice day!

@typescript-bot
Copy link
Contributor

typescript-bot commented Feb 7, 2019

A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

@typescript-bot
Copy link
Contributor

@SimonSchick - It appears Travis did not correctly run on this PR! /cc @RyanCavanaugh to investigate and advise.

@SimonSchick
Copy link
Contributor Author

@Flarna knowing the @types/node CI this will take a while to pass, feel free to leave comments :)

@typescript-bot
Copy link
Contributor

typescript-bot commented Feb 7, 2019

@SimonSchick The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@typescript-bot
Copy link
Contributor

@SimonSchick The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@typescript-bot
Copy link
Contributor

@SimonSchick The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@typescript-bot
Copy link
Contributor

typescript-bot commented Feb 8, 2019

@SimonSchick The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@typescript-bot
Copy link
Contributor

@SimonSchick The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@SimonSchick
Copy link
Contributor Author

Ok, having some problems with TextEncoder, mostly packages loading dom and node typings.

@typescript-bot
Copy link
Contributor

typescript-bot commented Feb 8, 2019

@SimonSchick The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@Flarna
Copy link
Contributor

Flarna commented Feb 8, 2019

I think it's a good idea to remove obsolete type definitions but I would do this in a separate PR just in case someone complains and a revert is needed. CI for node was green yesterday so maybe a good time window today.

@SimonSchick
Copy link
Contributor Author

@Flarna problem is that TextEncoder conflicts with dom typings now, I don't see how I can fix this :/

@Flarna
Copy link
Contributor

Flarna commented Feb 8, 2019

I think #25342 was about a similar problem with URL which was added to global. Solution that time was to simply skip this and it seems no one complained till now :o).

In general I recommend to keep using module dom in tests here as any incompatibilities with dom result in a lot broken builds in the wild.

@SimonSchick
Copy link
Contributor Author

@Flarna yea that's what I thought too, problem is that this really isn't a sustainable solution.

@typescript-bot typescript-bot added Owner Approved A listed owner of this package signed off on the pull request. Merge:Express and removed Awaiting reviewer feedback labels Feb 10, 2019
@SimonSchick
Copy link
Contributor Author

@btoueg @Flarna merged changes from #32597 into this PR.
Also:
Cleaned up crypto file by introducing Binary and BinaryLike and KeyLike types to reduce type union bloat.

@typescript-bot
Copy link
Contributor

typescript-bot commented Feb 11, 2019

@SimonSchick The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@Flarna
Copy link
Contributor

Flarna commented Feb 11, 2019

Merge seems to be ok. I have no special opinion regarding the new crypto types.
But it seems the removal of crypto.Credentials results in an unhappy irc module.

Any idea why other node maintainer have not been invited for a review by @typescript-bot ?

@SimonSchick
Copy link
Contributor Author

@Flarna according to the docs crypto.Credentials was wrong, unfortunately I can't run the node depedency tests locally, will fix soon. :)

@minestarks minestarks merged commit 5f59f3d into DefinitelyTyped:master Feb 12, 2019
@typescript-bot
Copy link
Contributor

I just published @types/irc@0.3.33 to npm.

@typescript-bot
Copy link
Contributor

I just published @types/node@11.9.0 to npm.

@typescript-bot
Copy link
Contributor

I just published @types/node@10.12.25 to npm.

@qm3ster
Copy link

qm3ster commented Feb 13, 2019

Thank you for the toJSON(): { type: 'Buffer', data: number[] };
Shouldn't the encoding argument in Buffer.write be the BufferEncoding union declared above instead of string? (I mentioned this as an also in #32926, not sure if anyone saw)

@SimonSchick
Copy link
Contributor Author

@qm3ster I guess so, would also make sense to create separate overloads for all other functions that take encoding as it's only required for strings.
Please create a new issue for this, not sure when I will get around to do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Definition This PR creates a new definition package. Owner Approved A listed owner of this package signed off on the pull request. Popular package This PR affects a popular package (as counted by NPM download counts).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@types/node Buffer Definition for "RemoteInfo" and related functions in dgram for Node
8 participants