Split node into one file per module #32567

rbuckton · 2019-01-28T20:13:51Z

Per #30477 (comment) and #30477 (comment), this splits node's definitions into one file per module in advance of leveraging "typesVersions" support.

rbuckton · 2019-01-28T20:16:50Z

@RyanCavanaugh, @DanielRosenwasser: Do you have any thoughts on this change? This was recommended by @Flarna and @KonanMentor as something worth doing as part of adding "typesVersions" support to node.

typescript-bot · 2019-01-28T20:22:28Z

@rbuckton 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.

Flarna · 2019-01-28T20:39:56Z

Honestly speaking I'm still not sure if typesversion is something we want to have at all as it just increases the maintenance burden - it's still needed to type everything for 2.1. Personally I would prefer to raise the minimum typescript version like it was done in other modules (e.g. react used min version 2.8). Are there any plans to move forward in this?

Currently the use if it blocks node changes anyway as CI fails in jsdom and since a few hours additionally in google-cloud__tasks (see #32444).

If the decisions is to use typesversions splitting files is a good idea in my opinion. Most likely tests need to be splitted also as they will differ also between typescript version once the definitions itself are version specific.

It would be also good to have a better separation between the subfolders created for typescript versions and that ones created because of node versions.

SimonSchick

I tried this a few months ago but ran into a few roadblocks (tests weren't able to import modules for some reason), good job!

This will make maintaining these typings drastically easier!

Some notes:
We should probably try to avoid defining modules in global and instead try to have global import those from the respective module definitions, see module, process, etc.
Optional: We can probably cleanup imports a fair bit, eg. replace wildcard imports with more specific ones.

SimonSchick · 2019-01-28T20:36:13Z

types/node/process.d.ts

@@ -0,0 +1,3 @@
+declare module "process" {
+    export = process;


Nit: Shouldn't this file contain the actual process definitions and the global should reference this?

Its a bit tricky to define it that way, since you cant import a module into the global scope. You can add a global augmentation inside the module, however:

declare module "process" { global { var process: NodeJS.Process; ... } export = process; }

However I don't want to dramatically change the shape of the NodeJS API in this PR.

That's fine.

SimonSchick · 2019-01-28T20:36:25Z

types/node/module.d.ts

@@ -0,0 +1,3 @@
+declare module "module" {
+    export = NodeJS.Module;


See comment at module.d.ts

rbuckton · 2019-01-28T20:44:54Z

Unfortunately, "typesVersions" is the only way that we can fix the node definitions to allow us to make changes to core lib definitions in the compiler that would conflict with the forward definitions in the node package. Even if we move the minimum TypeScript version forward to 2.8 we would still have this issue as some of the core lib definition changes we would like to make require features not present until at least 3.0.

We can discuss this more within the TypeScript team, but the folder structure we are using now is what we've decided on and are currently enforcing in both dtslint and types-publisher.

SimonSchick · 2019-01-28T20:46:30Z

@Flarna is 2.x even officially supported anymore eg. is 2.x on LTS?
I cannot find any information on this.

SimonSchick · 2019-01-28T20:51:10Z

Also, should test files be split too?

SimonSchick · 2019-01-28T20:58:04Z

Also once this PR lands I will look into updating typings for node v11.

typescript-bot · 2019-01-28T21:22:56Z

@rbuckton 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 · 2019-01-31T11:47:48Z

I did a closer look into the changes and tried them in my local setup - it works.

I would also prefer to move at least parts from global.d.ts (e.g. NodeJS.process, NodeJS.ConsoleConstructor, NodeJS.Module into the corresponding module files and keep in globals only the real globals. If there is anything left a common.d.ts could be used but not sure if this is needed.

rbuckton · 2019-02-01T00:39:33Z

@Flarna: Should it matter that the definition for "process" in node is literally module.exports = process?

rbuckton · 2019-02-01T00:58:28Z

Also, should test files be split too?

If necessary we should do this in a follow-on PR.

typescript-bot · 2019-02-01T01:27:31Z

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

rbuckton · 2019-02-01T01:31:55Z

Tests look good, the only failing tests are due to the recent addition of readonly for arrays/tuples that is in the latest dev builds.

Flarna · 2019-02-01T12:05:55Z

Honestly speaking I don't know why the definition of node (e.g. use namespace NodeJS,..) is done that way. I guess because it was created before typescript 2.0 even existed and at that time some augmentations of e.g. classes or so were not possible.
As there are now a lot modules relying on this namespace it's anyway mostly impossible to improve/change this.
What could be done is to move e.g. process specific parts of namespace NodeJS to the process.d.ts file.

rbuckton · 2019-02-01T22:45:59Z

@Flarna, @SimonSchick I may have to revert this change and go back to the previous approach to support "typesVersions", as this change seems to be causing problems for some users (see #32720).

SimenB · 2019-03-09T09:21:16Z

As there are now a lot modules relying on this namespace it's anyway mostly impossible to improve/change this.

Right now it's also not really possible for libraries to move away from it either (from my understanding). E.g.

import {ReadStream} from 'tty';
import process from 'process';

const stream: ReadStream = process.stdin;

gets error TS2740: Type 'ReadStream' is missing the following properties from type 'ReadStream': connect, setTimeout, setNoDelay, setKeepAlive, and 17 more.

EDIT: Ergh, I guess this would solve that:

What could be done is to move e.g. process specific parts of namespace NodeJS to the process.d.ts file.

Are there any plans/issue for this? In order to start fixing e.g. jestjs/jest#8092 it seems we have to be able to move off of NodeJS, no? Or won't that help?

SimonSchick · 2019-03-09T09:44:55Z

I think the biggest painpoint/reason that this namespace exists was that global definitions cannot use imports from modules?

But yea, getting rid of the NodeJS namespace would be a big deal, we would have to wait for nodev12 minimum.

Split node into one file per module

4e27a4d

rbuckton mentioned this pull request Jan 28, 2019

[node] Adds a 'typesVersions' entry so that Node definitions can start leveraging new compiler features. #30477

Closed

6 tasks

rbuckton requested review from RyanCavanaugh and DanielRosenwasser January 28, 2019 20:16

typescript-bot added this to Waiting for Reviewers in Pull Request Status Board Jan 28, 2019

typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). Awaiting reviewer feedback labels Jan 28, 2019

SimonSchick approved these changes Jan 28, 2019

View reviewed changes

typescript-bot moved this from Waiting for Reviewers to Needs Author Attention in Pull Request Status Board Jan 28, 2019

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 Jan 28, 2019

typescript-bot added the Has Merge Conflict This PR can't be merged because it has a merge conflict. The author needs to update it. label Jan 29, 2019

Merge branch 'master' into nodeSplit

0542814

typescript-bot removed the Has Merge Conflict This PR can't be merged because it has a merge conflict. The author needs to update it. label Feb 1, 2019

SimonSchick approved these changes Feb 1, 2019

View reviewed changes

rbuckton merged commit 028e633 into master Feb 1, 2019

typescript-bot removed this from Needs Author Attention in Pull Request Status Board Feb 1, 2019

fearthecowboy mentioned this pull request Feb 1, 2019

Node split -- broke my typescript projects #32720

Closed

2 tasks

rbuckton deleted the nodeSplit branch June 10, 2019 19:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split node into one file per module #32567

Split node into one file per module #32567

rbuckton commented Jan 28, 2019

rbuckton commented Jan 28, 2019

typescript-bot commented Jan 28, 2019 •

edited

Flarna commented Jan 28, 2019

SimonSchick left a comment

SimonSchick Jan 28, 2019

rbuckton Feb 1, 2019

SimonSchick Feb 1, 2019

SimonSchick Jan 28, 2019

rbuckton commented Jan 28, 2019 •

edited

SimonSchick commented Jan 28, 2019

SimonSchick commented Jan 28, 2019

SimonSchick commented Jan 28, 2019

typescript-bot commented Jan 28, 2019 •

edited

Flarna commented Jan 31, 2019

rbuckton commented Feb 1, 2019

rbuckton commented Feb 1, 2019

typescript-bot commented Feb 1, 2019

rbuckton commented Feb 1, 2019

Flarna commented Feb 1, 2019

rbuckton commented Feb 1, 2019

SimenB commented Mar 9, 2019 •

edited

SimonSchick commented Mar 9, 2019

		@@ -0,0 +1,3 @@
		declare module "module" {
		export = NodeJS.Module;

Split node into one file per module #32567

Split node into one file per module #32567

Conversation

rbuckton commented Jan 28, 2019

rbuckton commented Jan 28, 2019

typescript-bot commented Jan 28, 2019 • edited

Flarna commented Jan 28, 2019

SimonSchick left a comment

Choose a reason for hiding this comment

SimonSchick Jan 28, 2019

Choose a reason for hiding this comment

rbuckton Feb 1, 2019

Choose a reason for hiding this comment

SimonSchick Feb 1, 2019

Choose a reason for hiding this comment

SimonSchick Jan 28, 2019

Choose a reason for hiding this comment

rbuckton commented Jan 28, 2019 • edited

SimonSchick commented Jan 28, 2019

SimonSchick commented Jan 28, 2019

SimonSchick commented Jan 28, 2019

typescript-bot commented Jan 28, 2019 • edited

Flarna commented Jan 31, 2019

rbuckton commented Feb 1, 2019

rbuckton commented Feb 1, 2019

typescript-bot commented Feb 1, 2019

rbuckton commented Feb 1, 2019

Flarna commented Feb 1, 2019

rbuckton commented Feb 1, 2019

SimenB commented Mar 9, 2019 • edited

SimonSchick commented Mar 9, 2019

typescript-bot commented Jan 28, 2019 •

edited

rbuckton commented Jan 28, 2019 •

edited

typescript-bot commented Jan 28, 2019 •

edited

SimenB commented Mar 9, 2019 •

edited