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

feat/types: add main and types fields #823

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 5 additions & 26 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -319,32 +319,7 @@ module.exports = {
};
```

The `options` object contains the following:

```tsx
export interface TsdxOptions {
// path to file
input: string;
// Name of package
name: string;
// JS target
target: 'node' | 'browser';
// Module format
format: 'cjs' | 'umd' | 'esm' | 'system';
// Environment
env: 'development' | 'production';
// Path to tsconfig file
tsconfig?: string;
// Is error extraction running?
extractErrors?: boolean;
// Is minifying?
minify?: boolean;
// Is this the very first rollup config (and thus should one-off metadata be extracted)?
writeMeta?: boolean;
// Only transpile, do not type check (makes compilation faster)
transpileOnly?: boolean;
}
```
The `options` object type definition in [src/externalTypes.ts](src/externalTypes.ts).

#### Example: Adding Postcss
agilgur5 marked this conversation as resolved.
Show resolved Hide resolved

Expand All @@ -354,6 +329,10 @@ const autoprefixer = require('autoprefixer');
const cssnano = require('cssnano');

module.exports = {
/**
* @param {import('tsdx').RollupOptions} config
* @param {import('tsdx').TsdxOptions} options
*/
rollup(config, options) {
config.plugins.push(
postcss({
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
"bugs": {
"url": "https://github.com/formium/tsdx/issues"
},
"typings": "./dist/externalTypes.d.ts",
"bin": {
"tsdx": "./dist/index.js"
},
Expand Down
2 changes: 2 additions & 0 deletions src/externalTypes.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export { RollupOptions } from 'rollup';
Copy link
Collaborator

@agilgur5 agilgur5 Aug 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, TSDX itself uses RollupOptions internally.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

export { TsdxOptions } from './types';
20 changes: 10 additions & 10 deletions src/types.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
interface SharedOpts {
// JS target
/** JS target */
target: 'node' | 'browser';
// Path to tsconfig file
/** Path to tsconfig file */
tsconfig?: string;
// Is error extraction running?
/** Is error extraction running? */
extractErrors?: boolean;
}

Expand Down Expand Up @@ -33,19 +33,19 @@ export interface NormalizedOpts
}

export interface TsdxOptions extends SharedOpts {
// Name of package
/** Name of package */
name: string;
// path to file
/** path to file */
input: string;
// Environment
/** Environment */
env: 'development' | 'production';
// Module format
/** Module format */
format: ModuleFormat;
// Is minifying?
/** Is minifying? */
minify?: boolean;
// Is this the very first rollup config (and thus should one-off metadata be extracted)?
/** Is this the very first rollup config (and thus should one-off metadata be extracted)? */
writeMeta?: boolean;
// Only transpile, do not type check (makes compilation faster)
/** Only transpile, do not type check (makes compilation faster) */
transpileOnly?: boolean;
}

Expand Down
4 changes: 4 additions & 0 deletions test/integration/fixtures/build-withConfig/tsdx.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@ const autoprefixer = require('autoprefixer');
const cssnano = require('cssnano');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


module.exports = {
/**
* @param {import('tsdx').RollupOptions} config
* @param {import('tsdx').TsdxOptions} options
Comment on lines +7 to +8
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*/
rollup(config, options) {
config.plugins.push(
postcss({
Expand Down
2 changes: 1 addition & 1 deletion tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"importHelpers": true,
"esModuleInterop": true,
"outDir": "dist",
"declaration": false,
"declaration": true,
agilgur5 marked this conversation as resolved.
Show resolved Hide resolved
"module": "commonjs",
"rootDir": "src",
"strict": true,
Expand Down