feat/types: add main and types fields #823

cncolder · 2020-08-23T06:46:57Z

generate type difinition files
tsdx is requireable now
- so user can put TsdxOptions into tsdx.config.js JSDoc

vercel · 2020-08-23T06:47:03Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/formium/tsdx/l67cmftp5
✅ Preview: https://tsdx-git-types.formium.vercel.app

agilgur5

This exposes a lot of private APIs that it definitely shouldn't. If you'd like to use TsdxOptions, should use an external typing for it instead of exposing private APIs.

I'm also not sure exposing TsdxOptions itself is that great of an idea given that there's a big warning about how brittle tsdx.config.js is.

Also this isn't a fix, there wasn't anything broken, this is a feature

package.json

cncolder · 2020-08-23T09:24:12Z

I copy TsdxOptions into externalTypes.ts. So user cannot explore more types in editor. e.g. Cmd+Click in vscode.

Update comments to JSDoc. So comments will emit into externalTypes.d.ts

I'm not sure if we need exclude another .d.ts files from npm. I wish leave there. It's cool if someone find code suggestions when require('tsdx/dist/createJestConfig')

agilgur5

Great work @cncolder! JSDoc comments are definitely nice to have. I think the functional changes are mostly good now and just some improvements to be made now.

Outside of the comments below, there's some other changes that should be made along with this:

updating the tsdx.config.js example in the REAMDE to use imports and JSDoc types. Also replace the TsdxOptions snippet there to a link to the typing file
updating the integration tests to use imports and JSDoc types

Also had a question on the imports. You took a type out of Rollup in #822 -- did you have to install Rollup directly as a devDep for that or did it work as a transitive dep? Was also thinking it might be better to export a TSDX type of the Rollup config since we could infer more

tsconfig.json

src/externalTypes.ts

cncolder · 2020-08-30T10:28:08Z

So now. What's your favor @agilgur5 ?

Active "declaration" in tsconfig.json. Then re-export TsdxOptions from types.ts.
Disable "declaration". And add a new file to npm like types/index.d.ts. But we still need main field in package.json

agilgur5

So now. What's your favor @agilgur5 ?

Yep that's the question I had when I was reviewing too. I'm not sure, probably fine with either. The public API is the same either way. Rollup itself uses 2 to simplify the API

agilgur5 · 2020-08-30T20:31:32Z

test/integration/fixtures/build-withConfig/tsdx.config.js

@@ -1,8 +1,14 @@
+/* eslint-disable-next-line no-unused-vars */


should add this to the example in the README too since they both use tsdx lint

tsdx lint will skip tsdx.config.js

README.md

src/externalTypes.ts

agilgur5 · 2020-08-30T20:41:54Z

src/externalTypes.ts

+  transpileOnly?: boolean;
+}
+
+export { RollupOptions } from 'rollup';


I was thinking of using the exact type that TSDX modifies this to, although that may not be possible to override

Nevermind, TSDX itself uses RollupOptions internally.

So I ran tsc over a tsdx.config.js with the proper import and a // @ts-check and allowJs: "true" etc and that actually failed because of the lack of exact typing. E.g.:

$ tsc --noEmit tsdx.config.js:13:5 - error TS2532: Object is possibly 'undefined'. 13 config.plugins.push(

Now the problem is that I'm not entirely sure how to do this in TS or if it's possible to. TSDX's Rollup config should conform to RollupOptions but should also be more specific than RollupOptions -- e.g. we should know that config.plugins.push is possible without any type-casting.

Of course, TSDX Rollup config could have its own interface that extends RollupOptions, but that would be the naive route and could still cause users some type-checking problems. The config is complex enough that I'd like it to be inferred (especially the plugins' types, since users may override them) while still conforming to RollupOptions (so we don't get problems like #371 ). I'm not sure if that's possible in TS or if I just haven't been able to figure out how to do it in TS.

agilgur5

I'm working on a refactor of some of the types, and in the process seem to have found a better usage

README.md

agilgur5 · 2020-10-01T04:19:44Z

src/externalTypes.ts

+  transpileOnly?: boolean;
+}
+
+export { RollupOptions } from 'rollup';


Nevermind, TSDX itself uses RollupOptions internally.

- generate type difinition files - tsdx is requireable now - so user can put `TsdxOptions` into tsdx.config.js JSDoc

- copy TsdxOptions to externalTypes.ts - so user cannot explore another types - use typings field same as basic template

- use JSDoc comments - so it will emit into externalTypes.d.ts

- update readme tsdx config example - update integration test fixtures

- re-export options type - update types comment to JSDoc - update readme options point to source code file

- remove main field from package.json - update integration test fixtures - update readme example

agilgur5

Thanks for all the iterations despite the long wait in between! I think this is close to complete, but I've found more issues as I've dug into it and tried more myself 😐 . This is definitely useful for tsdx.config.js users!

I could take this over completing this myself now though (you'd of course be credited regardless for this work!), especially as some of these issues likely require some internal typing refactoring (which I've already started too), but if you know more about some TS nuances in the comments, that could be quite helpful!

agilgur5 · 2020-10-12T07:09:43Z

src/externalTypes.ts

+  transpileOnly?: boolean;
+}
+
+export { RollupOptions } from 'rollup';


So I ran tsc over a tsdx.config.js with the proper import and a // @ts-check and allowJs: "true" etc and that actually failed because of the lack of exact typing. E.g.:

$ tsc --noEmit tsdx.config.js:13:5 - error TS2532: Object is possibly 'undefined'. 13 config.plugins.push(

Now the problem is that I'm not entirely sure how to do this in TS or if it's possible to. TSDX's Rollup config should conform to RollupOptions but should also be more specific than RollupOptions -- e.g. we should know that config.plugins.push is possible without any type-casting.

Of course, TSDX Rollup config could have its own interface that extends RollupOptions, but that would be the naive route and could still cause users some type-checking problems. The config is complex enough that I'd like it to be inferred (especially the plugins' types, since users may override them) while still conforming to RollupOptions (so we don't get problems like #371 ). I'm not sure if that's possible in TS or if I just haven't been able to figure out how to do it in TS.

agilgur5 · 2020-10-12T07:11:25Z

test/integration/fixtures/build-withConfig/tsdx.config.js

+   * @param {import('tsdx').RollupOptions} config
+   * @param {import('tsdx').TsdxOptions} options


Suggested change

* @param {import('tsdx').RollupOptions} config

* @param {import('tsdx').TsdxOptions} options

* @param {import('../../../../').RollupOptions} config

* @param {import('../../../../').TsdxOptions} options

You had require('..') before, so it should either be that ('..') or this, which works in VSCode but may not work during the test phase where this gets moved to the "staging" dir. But tsdx build can't type-check this anyway (otherwise we'd be using tsdx.config.ts). Might add tsc tests in the future for things like #871 though...

agilgur5 · 2020-10-12T07:14:50Z

test/integration/fixtures/build-withConfig/tsdx.config.js

@@ -3,6 +3,10 @@ const autoprefixer = require('autoprefixer');
 const cssnano = require('cssnano');


Can't comment at the top of the file, but per my other comment, we probably want to add // @ts-check to the top of this and the example file.

Adding allowJs + checkJs to templates' and the tests' tsconfig probably makes more sense long-term, but there will probably be some ramifications to that and may want to wait to do that until #871 is fixed and creates two tsconfigs, one for build and one for type-checking. I definitely need to test that more before adding.

vercel bot deployed to Preview August 23, 2020 06:47 View deployment

agilgur5 requested changes Aug 23, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

agilgur5 changed the title ~~(fix/types): add main and types fields (#822)~~ feat/types: add main and types fields (#822) Aug 23, 2020

agilgur5 changed the title ~~feat/types: add main and types fields (#822)~~ feat/types: add main and types fields Aug 23, 2020

agilgur5 linked an issue Aug 23, 2020 that may be closed by this pull request

Add TSDX's own typedefs in order to use in tsdx.config.js with JSDoc #822

Open

vercel bot deployed to Preview August 23, 2020 09:21 View deployment

vercel bot deployed to Preview August 23, 2020 09:38 View deployment

agilgur5 reviewed Aug 25, 2020

View reviewed changes

tsconfig.json Show resolved Hide resolved

src/externalTypes.ts Outdated Show resolved Hide resolved

vercel bot deployed to Preview August 30, 2020 10:46 View deployment

agilgur5 reviewed Aug 30, 2020

View reviewed changes

vercel bot deployed to Preview September 1, 2020 11:55 View deployment

agilgur5 mentioned this pull request Sep 16, 2020

fix: declaration is actually false by default microsoft/TypeScript-Website#1092

Merged

agilgur5 reviewed Oct 1, 2020

View reviewed changes

vercel bot deployed to Preview October 1, 2020 12:32 View deployment

agilgur5 added this to the v0.14.x milestone Oct 5, 2020

cncolder added 6 commits October 5, 2020 17:42

(fix/types): add main and types fields (jaredpalmer#822)

bbda469

- generate type difinition files - tsdx is requireable now - so user can put `TsdxOptions` into tsdx.config.js JSDoc

feat/types: export options type in isolated file

3c8c37d

- copy TsdxOptions to externalTypes.ts - so user cannot explore another types - use typings field same as basic template

feat/types: change externalTypes comments to jsdoc

77c0453

- use JSDoc comments - so it will emit into externalTypes.d.ts

feat/types: export rollup options type

26b2866

- update readme tsdx config example - update integration test fixtures

feat/types re-export options type

41c7fdf

- re-export options type - update types comment to JSDoc - update readme options point to source code file

feat/types: import types in jsdoc

ad4f04e

- remove main field from package.json - update integration test fixtures - update readme example

agilgur5 mentioned this pull request Oct 11, 2020

Node.js 13.14.0 'No "exports" main resolved in' @babel/helper-compilation-targets/package.json #900

Closed

agilgur5 reviewed Oct 12, 2020

View reviewed changes

agilgur5 force-pushed the types branch from dd6624d to ad4f04e Compare October 12, 2020 20:52

vercel bot deployed to Preview October 12, 2020 20:52 View deployment

agilgur5 mentioned this pull request Oct 20, 2020

Refactor/Improve some typings #909

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat/types: add main and types fields #823

feat/types: add main and types fields #823

cncolder commented Aug 23, 2020

vercel bot commented Aug 23, 2020 •

edited

Loading

agilgur5 left a comment •

edited

Loading

cncolder commented Aug 23, 2020 •

edited

Loading

agilgur5 left a comment

cncolder commented Aug 30, 2020

agilgur5 left a comment

agilgur5 Aug 30, 2020

cncolder Sep 1, 2020

agilgur5 Aug 30, 2020 •

edited

Loading

agilgur5 Oct 1, 2020

agilgur5 Oct 12, 2020

agilgur5 left a comment

agilgur5 Oct 1, 2020

agilgur5 left a comment •

edited

Loading

agilgur5 Oct 12, 2020

agilgur5 Oct 12, 2020

agilgur5 Oct 12, 2020

		@@ -1,8 +1,14 @@
		/* eslint-disable-next-line no-unused-vars */

		* @param {import('tsdx').RollupOptions} config
		* @param {import('tsdx').TsdxOptions} options

		@@ -3,6 +3,10 @@ const autoprefixer = require('autoprefixer');
		const cssnano = require('cssnano');

feat/types: add main and types fields #823

Are you sure you want to change the base?

feat/types: add main and types fields #823

Conversation

cncolder commented Aug 23, 2020

vercel bot commented Aug 23, 2020 • edited Loading

agilgur5 left a comment • edited Loading

Choose a reason for hiding this comment

cncolder commented Aug 23, 2020 • edited Loading

agilgur5 left a comment

Choose a reason for hiding this comment

cncolder commented Aug 30, 2020

agilgur5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agilgur5 Aug 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agilgur5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agilgur5 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Aug 23, 2020 •

edited

Loading

agilgur5 left a comment •

edited

Loading

cncolder commented Aug 23, 2020 •

edited

Loading

agilgur5 Aug 30, 2020 •

edited

Loading

agilgur5 left a comment •

edited

Loading