Skip to content

[@types/node] Add close event to FSWatcher#40912

Merged
orta merged 1 commit intoDefinitelyTyped:masterfrom
JIACHENG9:node/fs
Dec 10, 2019
Merged

[@types/node] Add close event to FSWatcher#40912
orta merged 1 commit intoDefinitelyTyped:masterfrom
JIACHENG9:node/fs

Conversation

@JIACHENG9
Copy link
Contributor

@JIACHENG9 JIACHENG9 commented Dec 9, 2019

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.

Event: 'close'#
Added in: v10.0.0
Emitted when the watcher stops watching for changes. The closed fs.FSWatcher object is no longer usable in the event handler.

https://nodejs.org/dist/latest-v13.x/docs/api/fs.html#fs_event_close
https://nodejs.org/dist/latest-v12.x/docs/api/fs.html#fs_event_close
https://nodejs.org/dist/latest-v11.x/docs/api/fs.html#fs_event_close
https://nodejs.org/dist/latest-v10.x/docs/api/fs.html#fs_event_close

@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). Awaiting reviewer feedback labels Dec 9, 2019
@typescript-bot
Copy link
Contributor

typescript-bot commented Dec 9, 2019 •
edited
Loading

@JIACHENG9 Thank you for submitting this PR!

🔔 @microsoft @DefinitelyTyped @jkomyno @a-tarasyuk @alvis @r3nya @btoueg @BrunoScheufler @smac89 @tellnes @Touffy @DeividasBakanas @eyqs @Flarna @Hannes-Magnusson-CK @KSXGitHub @hoo29 @kjin @ajafff @islishude @mwiktorczyk @mohsen1 @n-e @octo-sniffle @galkin @parambirs @eps1lon @SimonSchick @ThomasdenH @WilcoBakker @wwwy3y3 @ZaneHannanAU @samuela @kuehlein @j-oliveras @bhongy @chyzwar @trivikr @nguymin4 @yoursunny @qwelias @jeremiergz - 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.

galkin
galkin suggested changes Dec 9, 2019
View reviewed changes
Copy link
Contributor

@galkin galkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JIACHENG9, thank you for your work!
Could you remove changes at Node.js v11. We don't update definitions for EOL versions.

@JIACHENG9
Copy link
Contributor Author

JIACHENG9 commented Dec 9, 2019

Could you remove changes at Node.js v11.

@galkin, Done

galkin
galkin suggested changes Dec 9, 2019
View reviewed changes
Copy link
Contributor

@galkin galkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JIACHENG9, you have removed v10 too, but Node.js v10 will be supported until April 2021. V11 is already EOL. More details here https://github.com/nodejs/Release#end-of-life-releases

Please revert back changes v10

@JIACHENG9
Copy link
Contributor Author

JIACHENG9 commented Dec 9, 2019

https://github.com/nodejs/Release#end-of-life-releases

@galkin, Done

@typescript-bot typescript-bot added Owner Approved A listed owner of this package signed off on the pull request. The Travis CI build failed and removed Awaiting reviewer feedback labels Dec 9, 2019
@typescript-bot
Copy link
Contributor

typescript-bot commented Dec 9, 2019 •
edited
Loading

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

@JIACHENG9
Copy link
Contributor Author

JIACHENG9 commented Dec 9, 2019

@galkin The problem of ci error seems to have nothing to do with this pr. Could you re-run ci?

@galkin
Copy link
Contributor

galkin commented Dec 9, 2019

@JIACHENG9, i can not, but you can reopen the pull. If problem will be happen again, then we will ask TS core team member to merge with red test or help with fix.

@JIACHENG9 JIACHENG9 closed this Dec 9, 2019
@JIACHENG9 JIACHENG9 reopened this Dec 9, 2019
@typescript-bot
Copy link
Contributor

typescript-bot commented Dec 9, 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

typescript-bot commented Dec 9, 2019

👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings.

Let’s review the numbers, shall we?

node/v12

Comparison details for node/v12 📊
master #40912 diff
Batch compilation
Memory usage (MiB) 123.2 125.3 +1.7%
Type count 21725 21730 0%
Assignability cache size 39468 39468 0%
Language service
Samples taken 1871 1871 0%
Identifiers in tests 11757 11757 0%
getCompletionsAtPosition
    Mean duration (ms) 590.8 588.2 -0.4%
    Mean CV 7.8% 7.8%
    Worst duration (ms) 793.6 769.3 -3.1%
    Worst identifier SSL_OP_NO_TLSv1_2 assert
getQuickInfoAtPosition
    Mean duration (ms) 584.5 583.1 -0.2%
    Mean CV 7.7% 7.7% +0.1%
    Worst duration (ms) 797.3 757.9 -4.9%
    Worst identifier num sock

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

node/v10

Comparison details for node/v10 📊
master #40912 diff
Batch compilation
Memory usage (MiB) 122.3 119.8 -2.0%
Type count 18104 18109 0%
Assignability cache size 34683 34683 0%
Language service
Samples taken 6296 6302 0%
Identifiers in tests 10200 10200 0%
getCompletionsAtPosition
    Mean duration (ms) 569.7 570.2 +0.1%
    Mean CV 0.0% 0.0%
    Worst duration (ms) 999.5 1040.4 +4.1%
    Worst identifier inspector Module
getQuickInfoAtPosition
    Mean duration (ms) 572.0 570.4 -0.3%
    Mean CV 0.0% 0.0%
    Worst duration (ms) 1131.1 1355.1 +19.8%
    Worst identifier inspector message

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

@typescript-bot typescript-bot added the Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. label Dec 9, 2019
@orta
Copy link
Collaborator

orta commented Dec 10, 2019

Looks like travis was having issues responding back to GitHub. It's green though, which is enough for me.

@orta orta merged commit e0052e8 into DefinitelyTyped:master Dec 10, 2019
@typescript-bot
Copy link
Contributor

typescript-bot commented Dec 10, 2019

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

@typescript-bot
Copy link
Contributor

typescript-bot commented Dec 10, 2019

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

@Flarna Flarna mentioned this pull request Dec 10, 2019
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

4 more reviewers

@r3nya r3nya r3nya approved these changes

@SimonSchick SimonSchick SimonSchick approved these changes

@galkin galkin galkin approved these changes

@Flarna Flarna Flarna approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Owner Approved A listed owner of this package signed off on the pull request. Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. Popular package This PR affects a popular package (as counted by NPM download counts).

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@JIACHENG9 @typescript-bot @galkin @orta @r3nya @SimonSchick @Flarna