Refactor/Improve some typings #909

agilgur5 · 2020-10-20T03:49:41Z

Description

Commits

types: add remaining external typings

added @types/babel__core, @types/babel__traverse, and
@types/lodash.merge and removed their declarations in env.d.ts
- also removed rollup-plugin-terser's declaration there because it
  has types built-in
- remaining actually don't have types per in-line comments
this improved type-checking allowed TS to do stricter checks and so
exposed a few type errors that were fixed as well
- @rollup/plugin-babel depends on @types/babel__core as well, so the
  declaration here had previously overridden it
  - custom had to be augmented as it's not an option of
    @rollup/plugin-babel
  - passPerPreset did too though... it's not in @types/babel__core
    but it is a (deprecated) Babel option
    - logs seemed to show it was false regardless of what was
      passed in though...
- Rollup's plugins now started complaining that false wasn't a
  valid Plugin either, so added a .filter(Boolean) but that didn't
  fully fix it so had to type-cast

refactor/fix: consistently use capital TSDX everywhere

Jared had asked in a review comment back in December to consistently
use capital TSDX (not tsdx) so this establishes that throughout the
rest of the codebase
- I don't think any of these lines/files/variables were written by me,
  so I'm just fixing up existing ones
this came up as I was refactoring typings, so wanted to make sure
everything was consistent as I went

refactor/types: explicitly name "at least one" types

refactor/types: add a type for RollupOptionsWithOutput

refactor/types: add a TSDXConfig type

another PR by a contributor exposes the types for tsdx.config.js in
order to be used in JSDoc, so this makes it so only a single type is
needed and exported

Review Notes

The passPerPreset augmentation for @rollup/plugin-babel / @types/babel__core that is mentioned above is pretty confusing. I decided to leave it as is for now so as to potentially change anything, but my logs seem to show it isn't used 😕 🤔 :

Babel log output:

PartialConfig {
  options:
   { sourceMaps: true,
     caller:
      { name: '@rollup/plugin-babel',
        supportsStaticESM: true,
        supportsDynamicImport: true,
        supportsTopLevelAwait: true },
     filename:
      '[redacted]/tsdx/stage-build-default/src/syntax/jsx-import/JSX-B.jsx',
     babelrc: false,
     configFile: false,
     passPerPreset: false,
     envName: 'test',
     cwd:
      '[redacted]/tsdx/stage-build-default',
     root:
      '[redacted]/tsdx/stage-build-default',
     plugins: [],
     presets: [] },
  babelignore: undefined,
  babelrc: undefined,
  config: undefined }
{ sourceMaps: true,
  caller:
   { name: '@rollup/plugin-babel',
     supportsStaticESM: true,
     supportsDynamicImport: true,
     supportsTopLevelAwait: true },
  filename:
   '[redacted]/tsdx/stage-build-default/src/syntax/jsx-import/JSX-B.jsx',
  babelrc: false,
  configFile: false,
  passPerPreset: false,
  envName: 'test',
  cwd:
   '[redacted]/tsdx/stage-build-default',
  root:
   '[redacted]/tsdx/stage-build-default',
  plugins:
   [ ConfigItem {
       value: [Function],
       options: {},
       dirname:
        '[redacted]/tsdx/stage-build-default',
       name: undefined,
       file: [Object] },
     ConfigItem {
       value: [Function: _default],
       options: {},
       dirname:
        '[redacted]/tsdx/stage-build-default',
       name: undefined,
       file: [Object] },
     ConfigItem {
       value: [Function],
       options: {},
       dirname:
        '[redacted]/tsdx/stage-build-default',
       name: undefined,
       file: [Object] },
     ConfigItem {
       value: [Function],
       options: [Object],
       dirname:
        '[redacted]/tsdx/stage-build-default',
       name: undefined,
       file: [Object] },
     ConfigItem {
       value: [Function],
       options: [Object],
       dirname:
        '[redacted]/tsdx/stage-build-default',
       name: undefined,
       file: [Object] } ],
  presets:
   [ ConfigItem {
       value: [Function],
       options: [Object],
       dirname:
        '[redacted]/tsdx/stage-build-default',
       name: undefined,
       file: [Object] } ] }
PartialConfig {
  options:
   { sourceMaps: true,
     caller:
      { name: '@rollup/plugin-babel',
        supportsStaticESM: true,
        supportsDynamicImport: true,
        supportsTopLevelAwait: true },
     filename:
      '[redacted]/tsdx/stage-build-default/src/syntax/jsx-import/JSX-B.jsx',
     babelrc: false,
     configFile: false,
     passPerPreset: false,
     envName: 'test',
     cwd:
      '[redacted]/tsdx/stage-build-default',
     root:
      '[redacted]/tsdx/stage-build-default',
     plugins: [],
     presets: [] },
  babelignore: undefined,
  babelrc: undefined,
  config: undefined }

Misc

This will be rebased in as each commit is independent and some have some lengthy messages.

- added @types/babel__core, @types/babel__traverse, and @types/lodash.merge and removed their declarations in env.d.ts - also removed rollup-plugin-terser's declaration there because it has types built-in - remaining actually don't have types per in-line comments - this improved type-checking allowed TS to do stricter checks and so exposed a few type errors that were fixed as well - @rollup/plugin-babel depends on @types/babel__core as well, so the declaration here had previously overridden it - `custom` had to be augmented as it's not an option of `@rollup/plugin-babel` - `passPerPreset` did too though... it's not in `@types/babel__core` but it is a (deprecated) Babel option - logs seemed to show it was `false` regardless of what was passed in though... - Rollup's `plugins` now started complaining that `false` wasn't a valid Plugin either, so added a `.filter(Boolean)` but that didn't fully fix it so had to type-cast

- Jared had asked in a review comment back in December to consistently use capital TSDX (not `tsdx`) so this establishes that throughout the rest of the codebase - I don't think any of these lines/files/variables were written by me, so I'm just fixing up existing ones - this came up as I was refactoring typings, so wanted to make sure everything was consistent as I went - rename variable TsdxOptions to TSDXOptions - rename babelPluginTsdx to babelPluginTSDX - and rename the file to this as well - and links to the file in the docs - clarify extractErrors error message to specify `tsdx build`, not just "tsdx" - use TSDX instead of `tsdx` in template docs - don't change contributing docs though, because in that one it's referring to the bin global or repo `tsdx`, which is correct

- previously I had used the `[typeX, ...typeX[]]` TS syntax in types, which means an array of at least one of those types - I actually had to look up how to do this when I originally wrote it and it even confused Shawn when I did - now I've looked at a few times and it takes me a second to realize what that means again, so hopefully this change makes it easier to understand the type at-a-glance and has less mental overhead

- `RollupOptions & { output: OutputOptions }` was used a few times in the codebase, so create a type makes this re-usable and consistent - so it doesn't have to be augmented everywhere in-line like that - and also documents its meaning

- another PR by a contributor exposes the types for `tsdx.config.js` in order to be used in JSDoc, so this makes it so only a single type is needed and exported - the exact type needed is the only exported basically - and that we're not re-exporting Rollup types directly

agilgur5

Mostly looks good to me. Mostly just straightforward renames.

The external types portion is the most potentially confusing bit, since the type-checking just ignored some Babel and Rollup stuff before due to the lack of declarations. Babel stuff is necessary and fairly straightforward at least.

A few things I'd like to take a closer look at though

agilgur5 · 2020-10-20T03:57:23Z

src/createRollupConfig.ts

@@ -213,6 +213,6 @@ export async function createRollupConfig(
          toplevel: opts.format === 'cjs',
          warnings: true,
        }),
-    ],
+    ].filter(Boolean) as Plugin[],


This is a simple one and doesn't change any behavior, but the type-cast seemed to have eroded some typing requirements that exist without it. E.g. Rollup requires each Plugin to have a name property, and some of these custom ones here do not.

Ahh think I need a type predicate here per microsoft/TypeScript#20812. I've been using these type-casts after filter like #54 did, but a type predicate / type guard is the right way to do it that I believe will prevent eroding of types

So I tried the below:

].filter((e): e is Plugin => Boolean(e))

but that didn't fully work either. It also eroded some types, like the name property requirement mentioned above 😕

With the TSDXOptions cast it didn't work because it doesn't seem to be able to infer the AtLeastOne portion 😕 Using enums didn't help with that either 😕
But I was able to fix some type erosion since the esm format one doesn't fit the type since it's missing env. Need to link those two together somehow...

For this Rollup config piece, I was able to rewrite the condition && plugin to ...(condition ? [plugin] : []) which I've done before elsewhere, and then that worked, but it is quite verbose... maybe I can abstract that into a function...

agilgur5 · 2020-10-20T03:59:45Z

src/types.ts

+export interface TSDXConfig {
+  rollup: (config: RollupOptions, options: TSDXOptions) => RollupOptions;
+}


This still has the same problem as in #823 (comment) . Should probably try being more specific here and in createRollupConfig.ts.

agilgur5 added 5 commits October 15, 2020 01:37

This comment was marked as spam.

Sign in to view

vercel bot deployed to Preview October 20, 2020 03:49 View deployment

agilgur5 commented Oct 20, 2020

View reviewed changes

agilgur5 added the scope: internal Changes only affect the internal API label Oct 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor/Improve some typings #909

Refactor/Improve some typings #909

agilgur5 commented Oct 20, 2020 •

edited

Loading

This comment was marked as spam.

agilgur5 left a comment

agilgur5 Oct 20, 2020

agilgur5 Oct 26, 2020

agilgur5 Oct 26, 2020 •

edited

Loading

agilgur5 Oct 20, 2020

Refactor/Improve some typings #909

Are you sure you want to change the base?

Refactor/Improve some typings #909

Conversation

agilgur5 commented Oct 20, 2020 • edited Loading

Description

Commits

types: add remaining external typings

refactor/fix: consistently use capital TSDX everywhere

refactor/types: explicitly name "at least one" types

refactor/types: add a type for RollupOptionsWithOutput

refactor/types: add a TSDXConfig type

Tags

Review Notes

Misc

This comment was marked as spam.

agilgur5 left a comment

Choose a reason for hiding this comment

agilgur5 Oct 20, 2020

Choose a reason for hiding this comment

agilgur5 Oct 26, 2020

Choose a reason for hiding this comment

agilgur5 Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

agilgur5 Oct 20, 2020

Choose a reason for hiding this comment

agilgur5 commented Oct 20, 2020 •

edited

Loading

agilgur5 Oct 26, 2020 •

edited

Loading