Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support TS in sub packages #390

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

TrySound
Copy link
Contributor

@TrySound TrySound commented May 9, 2020

Ref #389

Emitted all types to directory instead of single file.
This way each sub package can specify own "types" field.

Though there are problems

  • tsc do not emit types imports in dts files

I solved by manual prepending import from types

import { mat2, mat2d, mat3, mat4, quat, quat2, vec2, vec3, vec4, ReadonlyMat2, ReadonlyMat2d, ReadonlyMat3, ReadonlyMat4, ReadonlyQuat, ReadonlyQuat2, ReadonlyVec2, ReadonlyVec3, ReadonlyVec4 } from './types';
/**
 * Quaternion
 * @module quat
 */
/**
 * Creates a new identity quat
 *
 * @returns {quat} a new quaternion
 */
export function create(): quat;
  • tsc do not resolve reused function from another module so we end with
    this
export const fromValues: typeof vec4.fromValues;
// 2693: 'vec4' only refers to a type, but is being used as a value here.
  • even if we write all imports from another module we have a conflict
    between module names and types
import { ..., vec2, ... } from './types';
export const fromValues: typeof vec4.fromValues;

Ref toji#389

Emitted all types to directory instead of single file.
This way each sub package can specify own "types" field.

Though there are problems
- tsc do not emit types imports in dts files

I solved by manual prepending import from types

```
import { mat2, mat2d, mat3, mat4, quat, quat2, vec2, vec3, vec4, ReadonlyMat2, ReadonlyMat2d, ReadonlyMat3, ReadonlyMat4, ReadonlyQuat, ReadonlyQuat2, ReadonlyVec2, ReadonlyVec3, ReadonlyVec4 } from './types';
/**
 * Quaternion
 * @module quat
 */
/**
 * Creates a new identity quat
 *
 * @returns {quat} a new quaternion
 */
export function create(): quat;
```

- tsc do not resolve reused function from another module so we end with
this
```
export const fromValues: typeof vec4.fromValues;
// 2693: 'vec4' only refers to a type, but is being used as a value here.
```

- even if we write all imports from another module we have a conflict
between module names and types

```
import { ..., vec2, ... } from './types';
export const fromValues: typeof vec4.fromValues;
```
@hustcc
Copy link

hustcc commented May 11, 2020

Maybe this the minimum cost plan for sub-packages type define.

nit: But because the npm package files are generated by script, so we can only test the type define locally, this can easily lead to package quality problems. So test case for type define is suggested.

  1. build npm package
  2. the test file(.ts) should import dist folder
  3. tsc the test files, check whether there are type define errors

@stefnotch
Copy link
Collaborator

I've been looking at this again and it seems that if Rollup implements this rollup/rollup#2201 (Object shape tree-shaking), we wouldn't need the sub packages for tree-shaking anymore. Is that correct?

@TrySound
Copy link
Contributor Author

TrySound commented Jun 5, 2020

I didn't dig in this proposal. Can't say anything.

@stefnotch
Copy link
Collaborator

I think this would be the relevant webpack issue webpack/webpack#8057

@0ctothorp
Copy link

0ctothorp commented Aug 22, 2020

Hey, I just tried this in my project and it works great. Basically I just wanted this kind of imports to work well in typescript and this PR seems to do it just fine!

import { fromTranslation } from 'gl-matrix/mat4';

I would love to help push this forward if it's possible, but am not sure what problems are preventing it from being merged.

@stefnotch stefnotch added this to the 4.0 milestone Aug 22, 2020
@stefnotch
Copy link
Collaborator

Good question! The main thing preventing me from merging is that I currently don't have enough time to sort out the issues. I'm rather preoccupied with other projects.

@donmccurdy
Copy link

I think there's an easier way to solve this, by appending a few extra lines to the existing type definitions:

declare module 'gl-matrix/vec4' {
	import { vec4 } from 'gl-matrix';
	export = vec4;
}

declare module 'gl-matrix/vec3' {
	import { vec3 } from 'gl-matrix';
	export = vec3;
}

declare module 'gl-matrix/vec2' {
	import { vec2 } from 'gl-matrix';
	export = vec2;
}

declare module 'gl-matrix/mat4' {
	import { mat4 } from 'gl-matrix';
	export = mat4;
}

declare module 'gl-matrix/mat3' {
	import { mat3 } from 'gl-matrix';
	export = mat3;
}

^I've been using those in a global.d.ts file in my own project, but would be happy to submit them here if that sounds OK? Or have I missed part of the intent for this PR?

@stefnotch
Copy link
Collaborator

@donmccurdy Honestly, at this point, any solution that works would be super appreciated. So, a clear 👍 from my side.

@donmccurdy
Copy link

Hm, I tried putting that code (which works in a consuming project) into gl-matrix and there are some issues. TypeScript still needs separate files for each subpackage, and those subpackages must be located at the project root, e.g. ./mat4.d.ts or ./mat4/index.d.ts (see microsoft/TypeScript#8305). If polluting the project root is OK we could go ahead with that, or even generate and publish those files but omit them from versioning with .gitignore. Presumably that limitation applies to this PR, too.

If we want to avoid those root-level files or directories, TypeScript may begin supporting package exports around v4.4, and this would provide more flexible, explicit mappings to each entrypoint. Perhaps waiting for v4.4 is the way to go..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants