Proposal: guidance on modules/globals

[myitcv]

I would like to suggest an update to the best practices document, namely that we establish and document some sort of pattern, similar to React, for modules vs. global ambient declarations. The gist is very similar to my comments in #6112.

Idea being that hopefully:

• a greater percentage of new definitions will be ES6 module 'ready' right out of the box
• updates to existing definitions can easily cite the best practices on the motivation/style for the change being proposed in a PR that seeks to make a definition ES6 module 'ready', e.g. like Create lodash-global for global usage #6112

I've cited the React pattern in #6112 on the basis it seems like the right approach to follow. lodash I believe requires a slightly different pattern because we are effectively exporting a variable and a namespace (hence can't use export =).

The one question I would add is whether we can improve the test file situation with this pattern: in the React package, the only difference between react-tests.ts and react-global-tests.ts is as follows:

$ diff -wu react-tests.ts react-global-tests.ts
--- react-tests.ts      2015-09-09 15:29:36.595420049 +0100
+++ react-global-tests.ts       2015-09-09 15:29:36.595420049 +0100
@@ -1,5 +1,4 @@
-/// <reference path="react.d.ts" />
-import React = require("react");
+/// <reference path="react-global.d.ts" />

 interface Props extends React.Props<MyComponent> {
     hello: string;

It seems a burden to have to maintain both, with the difference being a single reference and a single import.

Would welcome your thoughts @borisyankov @vvakame @johnnyreilly @basarat

[myitcv]

Relevant discussion here too

[jbrantly]

Related: microsoft/TypeScript#4337

[jbrantly]

Yea I'm not sure how to fix this, but I agree it's crappy. Seems like the best solution would be to have support in the test runner to dynamically switch out what gets imported (some kind of preprocessor directive, maybe?).

Alternatively we could only test the module version or the namespace version but not both. That's how React was for a long time, I added the namespace tests just recently for consistency but definitely not happy about duplication.

[myitcv]

support in the test runner to dynamically switch out what gets imported

Agreed.

some kind of preprocessor directive, maybe?

Not sure we need something as heavyweight as support in the language...

We could do this via some sort of special comment scheme. There's already good support for this.

e.g. the following is node-uuid-test.ts (i.e. the default)

/* switch-zone-start */
/// <reference path="node-uuid.d.ts" />

import * as uuid from "node-uuid";
/* switch-zone-end */

// tests follow per https://github.com/myitcv/DefinitelyTyped/blob/node_uuid_es6_module/node-uuid/node-uuid-tests.ts

Then for every other file node-uuid-*.d.ts the equivalent node-uuid-*-tests.ts.template is consulted for content to switch into the switch zone. e.g. node-uuid-global-tests.ts.template:

/// <reference path="node-uuid-global.d.ts" />

// no import is needed

Then tests run.

Thoughts?

[jbrantly]

Not sure we need something as heavyweight as support in the language...

No, but it's related. We could potentially take cues from that in case language support was coming. I mean what you suggested with comments are essentially preprocessor directives.

In any case, I think we're on the same page minus implementation specifics. The basic idea being that the test runner would need to do something special involving scanning and manipulating the test files.

I would suggest maybe even a simpler scheme not involving .template files:

/* test: global */
/// <reference path="node-uuid-global.d.ts" />
/* test-end */

/* test: module */
/// <reference path="node-uuid.d.ts" />
import * as uuid from "node-uuid";
/* test-end */

/// ... tests ....

Where each test file is simply run multiple times for each occurrence of /* test: X */

[orta]

Hi thread, we're moving DefinitelyTyped to use GitHub Discussions for conversations the @types modules in DefinitelyTyped.

To help with the transition, we're closing all issues which haven't had activity in the last 6 months, which includes this issue. If you think closing this issue is a mistake, please pop into the TypeScript Community Discord and mention the issue in the definitely-typed channel.