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

Add types for experimetal worker_threads module #31177

Merged

Conversation

BendingBender
Copy link
Contributor

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

If changing an existing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: https://nodejs.org/dist/latest-v10.x/docs/api/worker_threads.html
  • Increase the version number in the header if appropriate.
  • If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.

@typescript-bot typescript-bot added this to Needs Author Attention in Pull Request Status Board Dec 8, 2018
@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). The Travis CI build failed labels Dec 8, 2018
@typescript-bot
Copy link
Contributor

typescript-bot commented Dec 8, 2018

@BendingBender Thank you for submitting this PR!

🔔 @microsoft @DefinitelyTyped @jkomyno @a-tarasyuk @alvis @r3nya @BrunoScheufler @smac89 @tellnes @DeividasBakanas @eyqs @Flarna @Hannes-Magnusson-CK @KSXGitHub @hoo29 @kjin @ajafff @islishude @mwiktorczyk @matthieusieben @mohsen1 @n-e @octo-sniffle @parambirs @eps1lon @SimonSchick @ThomasdenH @WilcoBakker @wwwy3y3 @ZaneHannanAU @jeremiergz @samuela - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@typescript-bot
Copy link
Contributor

@BendingBender 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 Dec 8, 2018

@BendingBender 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 typescript-bot moved this from Needs Author Attention to Waiting for Reviewers in Pull Request Status Board Dec 9, 2018
@typescript-bot typescript-bot moved this from Waiting for Reviewers to Needs Author Attention in Pull Request Status Board Dec 9, 2018
@typescript-bot
Copy link
Contributor

typescript-bot commented Dec 9, 2018

@BendingBender 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!

@ZaneHannanAU
Copy link
Contributor

ZaneHannanAU commented Dec 9, 2018

Probably shouldn't use console, but idk.

Looks good to me otherwise.

We REALLY need to deprecate those broken packages.

@typescript-bot typescript-bot added the Owner Approved A listed owner of this package signed off on the pull request. label Dec 9, 2018
@BendingBender
Copy link
Contributor Author

BendingBender commented Dec 9, 2018

@ZaneHannanAU the broken package is a really strange one, it only breaks on typescript@next. This looks like a bug in the compiler, not in the package definitions.

@Flarna Flarna mentioned this pull request Dec 9, 2018
10 tasks
@Flarna
Copy link
Contributor

Flarna commented Dec 9, 2018

Well, unist-util-is is just two days old so maybe a bit early to deprecate it 😁.
I asked the author in #31084 what's wrong as CI was green there.

@ZaneHannanAU
Copy link
Contributor

… I think, at this point, we should have the CI check every package and not just what has been definitely effected.

But then that'll take forever.

… ugh, there're gonna be too many damn edge cases.

types/node/index.d.ts Outdated Show resolved Hide resolved
types/node/index.d.ts Show resolved Hide resolved
types/node/index.d.ts Show resolved Hide resolved
@BendingBender BendingBender force-pushed the node-worker-threads branch 2 times, most recently from 5dae445 to 4cd8491 Compare December 10, 2018 16:25
@typescript-bot
Copy link
Contributor

typescript-bot commented Dec 10, 2018

@BendingBender 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!

@amcasey
Copy link
Contributor

amcasey commented Dec 10, 2018

@Flarna I'm leaning towards a TS change as the cause, but I'm still investigating (cc @Rokt33r).

@amcasey
Copy link
Contributor

amcasey commented Dec 10, 2018

Changed my mind - I think the test is wrong. I'll follow up on the other PR (and try to determine why the CI didn't catch this).

@Flarna
Copy link
Contributor

Flarna commented Dec 12, 2018

@BendingBender uninst-util-is should be fixed by #31257. You could try to retrigger CI, hopefully it's green now.

@typescript-bot
Copy link
Contributor

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!

@jessetrinity jessetrinity merged commit ca42826 into DefinitelyTyped:master Dec 12, 2018
@typescript-bot typescript-bot removed this from Check and Merge in Pull Request Status Board Dec 12, 2018
@BendingBender BendingBender deleted the node-worker-threads branch December 12, 2018 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

9 participants