Prevent @types/node and @types/requirejs-confusion

[manuth]

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).

Select one of these and delete the others:

If changing an existing definition:

• Provide a URL to documentation or source code which provides context for the suggested changes:
  I created this PR as many people are having trouble with requirejs overriding the variable require:
  • Variable 'require' must be of type 'NodeRequire', but here has type 'Require' #31628
  • @types/requirejs Variable 'require' must be of type 'NodeRequire' #15851
  • Node and requirejs typings messing up my build rollup/rollup#2142
• 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" }.

About this PR

According to the documentation, @types/requirejs can be invoked through require() and requirejs().
As require() is declared by @types/node, too, which is part of most projects.

For that reason I decided to make @types/requirejs exporting requirejs() rather than require().

[typescript-bot]

👋 Hi there! I’ve run some quick performance metrics against master and your PR. This is still an experiment, so don’t panic if I say something crazy! I’m still learning how to interpret these metrics.

Let’s review the numbers, shall we?

These typings are for a package that doesn’t yet exist on master, so I don’t have anything to compare against yet! In the future, I’ll be able to compare PRs to requirejs with its source on master.

**Comparison details** 📊

Batch compilation
Type count	8964
Assignability cache size	32520
Subtype cache size	3
Identity cache size	3
Language service measurements
Samples taken	140
Identifiers in tests	28
getCompletionsAtPosition
Mean duration (ms)	315.8
Median duration (ms)	308.3
Std. deviation (ms)	49.8
Worst duration (ms)	365.3
Worst identifier	recOne
getQuickInfoAtPosition
Mean duration (ms)	312.8
Median duration (ms)	299.5
Std. deviation (ms)	50.4
Worst duration (ms)	373.1
Worst identifier	config
System information
Node version	v10.15.3
CPU count	2
CPU speed	2.294 GHz
CPU model	Intel(R) Xeon(R) CPU E5-2673 v4 @ 2.30GHz
CPU Architecture	x64
Memory	6.8 GiB
Platform	linux
Release	4.15.0-1045-azure

---

If you have any questions or comments about me, you can ping @andrewbranch. Have a nice day!

[typescript-bot]

@manuth Thank you for submitting this PR!

🔔 @jbaldwin - 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]

We've gotten sign-off from a reviewer 👏. A maintainer will soon review this PR and merge it if there are no issues. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for contributing to DefinitelyTyped!

[RyanCavanaugh]

This seems extremely dangerous in terms of breaking existing code and JS users. I would need to see data in terms of what % of extant code uses require vs requirejs to be OK publishing this.

[manuth]

Oh - I thought this should not damage any code as I increased the major version-number.

[manuth]

@RyanCavanaugh is this a wrong assumption?
Is there anything else I need to do in order to increase the major version-number but changing the file-header of index.d.ts?

[manuth]

So I just checked the guide of DefinitelyTyped and it says that there is nothing to do but increasing the version-number in the first line of the index.d.ts-file:
https://github.com/DefinitelyTyped/DefinitelyTyped#how-do-definitely-typed-package-versions-relate-to-versions-of-the-corresponding-library

I updated the major version which means that no package will be affected.
Is there anything more I have to do?

[Eagle94T]

@RyanCavanaugh what's the status for this PR?
With updating the major version, this shouldn't affect any existing projects.

[Eagle94T]

I'm sorry to ask again, but why is this PR still not merged?
I need this change, since I'm using aurelia-bundler (uses @types/node) and @types/select2 (uses @types/requirejs)

[andrewbranch]

Oh - I thought this should not damage any code as I increased the major version-number.

...is this a wrong assumption?

Yes—the version number is supposed to represent the version of the JavaScript library on npm. The latest release of RequireJS is 2.3.6. In fact, @sandersn just added a CI rule to prevent type definitions from referencing non-existent versions of their JS counterparts.

Echoing Ryan, I think we would want to see some data here. I think my hunch is that lots of RequireJS users probably use require() (and want to keep it that way), in which case this change is probably a non-starter, although well-intentioned. If you can prove me wrong, then we can proceed. 👍

[andrewbranch]

It appears that @typescript-bot might have interpreted my trailing “👍” in that last comment as an approval? I’m requesting changes just so it’s clear the status is not mergeable without further research/discussion.

[typescript-bot]

@manuth One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. Thank you!

[manuth]

Honestly I don't think "people may want to keep using require()" is a pretty bad argument.
By now you cannot write typescript-modules which have both @types/requirejs and @types/node in their dependency-tree which often might be the case as some types in DefinitelyTyped use either parts of @types/requirejs or @types/node.

To me its a bad idea to prevent people from writing typescript-code just because "people want to use require().

[sandersn]

Here's a workaround: provide a node-specific version of the package which basically implements this change, but copy it into a subdirectory named node. Then people who have the conflict can add the following path mapping to their tsconfig:

        "baseUrl": ".",
        "paths": {
            "requirejs": ["node_modules/@types/requirejs/node/index.d.ts"]
        }

It should be possible to share the types between the main file and the node file, so that they only differ in the last few lines -- the main file would have 3 globals and the node file would have a single export =.

[andrewbranch]

@sandersn I tried this locally, but node_modules/@types/requirejs/index.d.ts (with its ambient declarations) still get pulled into the program. I think you’d have to combine this approach with specifying typeRoots or types in your tsconfig, which could be pretty frustrating if you have tons of @types packages and you just need to prevent one file in them from being included. (Related: microsoft/TypeScript#18588)

[typescript-bot]

@manuth I haven't seen anything from you in a while and this PR currently has problems that prevent it from being merged. The PR will be closed tomorrow if there aren't new commits to fix the issues.

[typescript-bot]

@manuth To keep things tidy, we have to close PRs that aren't mergeable but don't have activity from their author. No worries, though - please open a new PR if you'd like to continue with this change. Thank you!

[manuth]

@RyanCavanaugh. @andrewbranch I'd be glad if you guys could discuss this further so I can open a new PR for resolving the node/requirejs-intersection.

[sandersn]

@andrewbranch I was wrong about my fix, the errors you saw got lost in a bunch of others. I'm out of ideas on this one.

[manuth]

Should I wait for you guys to work out a solution or should I rather open a new PR?

[andrewbranch]

My only idea is to publish a separate package, e.g. @types/requirejs-node.