[WIP] Typescript port

[koresar]

Wow! this is AWESOME!

[PopGoesTheWza]

TODO: [x] complete @stamp/arg-over-prop first pass of TS transform [x] complete @stamp/collision first pass of TS transform [x] add npm buildscript (and the relevant per package tsconfig.json (which should make Travis happy) [-] make .js files to be produced in the (new) dist folder (irrelevant) [-] make .d.ts files to be produced in the (new) types folder (irrelevant) [ ] consolidate and enhance common type signature, either in @stamp/is (which is the package every other one depends on) or a new and dedicated @stamp/typescript-typings (à la @microsoft/microsoft-graph-types) [x] adapt npm scripts lint & lint-fix if necessary [x] add npm script dryrun (see npm doc) if necessary and adapt npmignore if necessary

Note: I am not touching check-compose, and that's on purpose! ;)

[PopGoesTheWza]

@koresar This is likely something silly I did wrong... I broke the default exports upon which all tests rely on.

[koresar]

Sorry, my expertise is low in this area.

[PopGoesTheWza]

Hum... export = and import = require()

The problem

So current @stamp packages use the classic default export CommonJS syntax:

const anyName = require("thisModuleOnlyExportASingleDefault");

module.exports = somethingAsDefaultExport

This syntax is not compatible with what Typescript typical/recommended syntax produces:

export

export const SomeNamedExport
export default SomeNamedExport // effectively exported with the name 'default'

Which is roughly compiled into the following javascript:

Object.defineProperty(exports, "__esModule", { value: true });
exports.SomeNamedExport;
exports.default = exports.SomeNamedExport;

import

import { compose } from '@stamp/compose'; // import named export
import nameForMyDefaultImport from '@stamp/compose'; // import default

Which is roughly compiled into the following javascript:

const compose_1 = require("@stamp/compose");

Where compose_1 is generated at compilation and each reference to compose becomes compose_1.compose. Default imports are resolved in a similar fashion but uses the value of compose_1.default

Options

Thus there is a dilemma (with possibly more than two alternatives):

1. break things and have @stamp packages import/export compatible with what Typescript produces (and marginally have a syntax compatible with esmodules)
2. write typescript code so that the compiled js remains compatible with current interface
3. limit typescript support to manually crafted/edited/maintained .d.ts files

Break import convention

The first option impacts all the existing code base but is rather straightforward.

Any default import from a @stamp module...

var compose = require('../packages/compose');

... should be edited so that it uses the default property from the object returned by require()

var compose = require('../packages/compose').default;

Alternatively, since the typescript code in this PR also as named exports, one could also use this syntax:

var { compose } = require('../packages/compose');

All __test__ files have been edited in order to correctly require modules compiled from typescript.

Twist typescript to remain compatible

The second option requires juggling with syntax in ugly ways and make the use of type definition clumsy. For example isFunction and isStamp from @stamp/is

[...]

/**
 * @internal Checks if passed argument is considered a `function`.
 */
export type isFunction = typeof isFunction;
const isFunction = <T extends Function = SomeFunction>(value: unknown): value is T => typeof value === 'function';

module.exports = isFunction;

Here the isFunction function is set as default export (module.exports =) which is not understood/supported by Typescript. The isFunction typing needs to be defined and exported separately.

import { isFunction } from './function';

const isfunction: isFunction = require('./function');
[...]

Now to use isFunction is @stamp/is/stamp.ts the function and the typing have to be imported distinctly (note that they cannot share the same name!) in a very kludgy way.

[PopGoesTheWza]

What is the difference between Push and PR Travis?

edit SO answer

[PopGoesTheWza]

@koresar please review my comment above regarding export/import issues.

Current PR follows the first option and breaks compatibility with existing code source.

If you want, I can also push the compiled .js files if you want to inspect what the compiled code looks like.

[koresar]

Thanks for the thorough investigation.

Babel compiles to the exactly same .js - export default becomes module.exports.default. You have discovered the issue which forced me to write this project (and stampit) with CommonJS+ES5. Namely, the Babel code generation. This is why my moto is "less tooling, less configuration". Also, this is why the famous Sindre Sorhus also does not use Babel and ESLint.

But we have to have the best of both words: leave the packages compatible, and have them as TypeScript. And I believe this is a long solved problem.

This package https://github.com/sindresorhus/is is written using TS, and exports all the compatible things. See https://unpkg.com/@sindresorhus/is@1.2.0/dist/index.js

"use strict";
/// <reference lib="esnext"/>
/// <reference lib="dom"/>
Object.defineProperty(exports, "__esModule", { value: true });

...

exports.default = is;
// For CommonJS default export support
module.exports = is;
module.exports.default = is;
//# sourceMappingURL=index.js.map

This might work. Not sure how to achieve this using the tcs.

[PopGoesTheWza]

@koresar Yeepee... Travis says yes now.

As per the solution from Sindre Sorhus, it seems so ovious once you've seen it.

[PopGoesTheWza]

@koresar I enhanced the proposed workaround so that module.exports.default is not enumerable (limiting the pollution of exported objects)

export default compose;

// For CommonJS default export support
module.exports = compose;
Object.defineProperty(module.exports, 'default', { enumerable: false, value: compose });

[koresar]

Awesome work mate!

[PopGoesTheWza]

Hey! Are you sure we want it merged yet? There is still plenty of Doc related TODOs and Tipescript support is minimal at this stage.

[koresar]

Sorry for rushing! My mistake. It still says WIP. Oops

Feel free to create another PR.

On Thu., 9 Jan. 2020, 18:35 PopGoesTheWza, notifications@github.com wrote:

Hey! Are you sure we want it merged yet? There is still plenty of Doc related TODOs and Tipescript support is minimal at this stage.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/stampit-org/stamp/pull/77?email_source=notifications&email_token=AAMMEL3RKB6FD2ABJ3YCEZLQ43HVDA5CNFSM4KDMKGLKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIPJXXQ#issuecomment-572431326, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMMEL4DMXDP3KB5ENTP4BTQ43HVDANCNFSM4KDMKGLA .