plugins icon indicating copy to clipboard operation
plugins copied to clipboard

breaking(typescript): allow `noEmit` and `emitDeclarationOnly`

Open Harris-Miller opened this issue 3 years ago • 8 comments

Rollup Plugin Name: typescript

This PR contains:

  • [ ] bugfix
  • [ ] feature
  • [x] refactor
  • [ ] documentation
  • [ ] other

Are tests included?

  • [ ] yes (bugfixes and features will not be merged without tests)
  • [x] no -- will add once proposed changes are accepted

Breaking Changes?

  • [x] yes (breaking changes will not be merged unless absolutely necessary)
  • [ ] no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

Description

Why

Currently this plugin forced the compilerOptions noEmit and emitDeclarationOnly to be false. There is a note // Typescript needs to emit the code for us to work with.

This is false

The way that the plugin is written, the load hook looks for if typescript did transpilation for a file and returns that if found, null otherwise. This means for emitDeclarationOnly rollup will simply load from the output of the previous plugin or from the file system. This is ideal to mimic the following:

// package.json
{
  "scripts": {
    "build": "babel src --out-dir dist" 
    "typecheck": "tsc --emitDeclarationOnly"
  }
}

Where build uses babel to do all transpilation, including typescript via @babel/preset-typescript, and tsc is used to only emit declaration files.

Currently, rollup can more or less do this by running all code though @rollup/plugin-typescript first and then @rollup/plugin-babel, however with the forced emitDeclarationOnly: false being on, all the code is run through the typescript transpiler first. If @rollup/plugin-babel is set up to do to typescript, there isn't a need to actually do this

By allowing emitDeclarationOnly: true, @rollup/plugin-typescript is drastically faster! Since it only has to emit declaration file (which are emitted by rollup separately as "asset" files in the generateBundle step). On the large projects I tested this out at my work, I say initial builds drop from 20 seconds down to 2 seconds

Breaking?

I would consider this a breaking change but it fundamentally changes the behavior of the plugin, any user that has noEmit or emitDeclarationOnly set to true in their tsconfig.json for other purposes (eg jest) and are relying on the fact that those properties are forced to false will see unintended change in their build. Therefor this has to be a Breaking change

Tests

I haven't included any tests for this change yet, I wanted to gauge how accepting you will be of this change before putting in that effort (I'm very busy at the moment). I will go back and write proper tests if the owners accept the idea of this change for merge

Harris-Miller avatar Jun 15 '22 17:06 Harris-Miller

Thanks for the PR. This is going to require approval from @NotWoods

shellscape avatar Jun 16 '22 03:06 shellscape

I'm open to this but I think we should put it behind an option, since the behaviour is unexpected. Most users of the plugin aren't using babel to compile typescript.

If the code changes are minimal:

  • Add option transpilerMode: 'none' | 'full'

If the code changes are complex enough its almost a separate plugin:

  • Let's export a second plugin as a named export. I've been thinking of doing this for a "transpile only" mode w/out typechecking.

NotWoods avatar Jun 16 '22 05:06 NotWoods

@NotWoods the changes are simple, so the transpilerMode option route would be the way to go. I would suggest 4 options:

  • 'full' (default) - this is the current behavior, emits declaration and transpiled code
  • 'declarationOnly' - only emit declaration .d.ts files, do not transpile
  • transpileOnly - do not emit declaration .d.ts files, only emit transpiled code
  • 'none' - emit nothing, only display typecheck messages in console

How does that sound?

Harris-Miller avatar Jun 17 '22 01:06 Harris-Miller

That sounds good to me. Please make sure to document 'none' with an example use case in the readme

NotWoods avatar Jun 17 '22 07:06 NotWoods

@NotWoods sorry for the delay, been very busy at work. I made the recommended changes by adding a transpilerMode and updated the docs.

Still need to add tests, will have that done in a week or so

Harris-Miller avatar Jul 07 '22 04:07 Harris-Miller

This is a really necessary feature, because the tsc doesn't keep the directory structure, and the esbuild and vite don't support types declarations. If I use vite I shoud add this plugin but with this option: emitDeclarationOnly

NikolayMakhonin avatar Jul 26 '22 07:07 NikolayMakhonin

@Harris-Miller please add some tests for this

shellscape avatar Jul 28 '22 02:07 shellscape

@shellscape @NikolayMakhonin @NotWoods Hi everyone. Sorry for the delay in getting tests written for this feature. My company's go live is next Tuesday and I haven't had any time to focus on OSS for a bit. After that go live I'll be able to dedicate some time to this

Harris-Miller avatar Jul 28 '22 02:07 Harris-Miller

@NotWoods I began writing tests and I found that the solution of adding a transpilerMode would be a breaking change. To re-iterate what this was to do:

  • 'full' (default) - this is the current behavior, emits declaration and transpiled code
  • 'declarationOnly' - only emit declaration .d.ts files, do not transpile
  • transpileOnly - do not emit declaration .d.ts files, only emit transpiled code
  • 'none' - emit nothing, only display typecheck messages in console

Those changes essential change it so instead of forcing the compilerOptions noEmit: false and emitDeclarationOnly: false, we would dynamically set them via transpilerMode.

Full 'full' (default) to work we'd have to additionally force declaration: true. That would be a breaking change as it could override if the user has declaration: false in their tsconfig.json

transpilerMode isn't possible without a breaking change.

The more I look at this, I think just a simple dontForceEmit boolean to opt-out of the current noEmit: false and emitDeclarationOnly: false is really all that would be needed. It would hand back control over those options to the user's tsconfig.json without a breaking change.

I do still believe that forcing those options should be removed altogether, but that too would be a breaking change. So the idea of opt-in to that is a good middle ground.

I'll have new code for that tomorrow

Harris-Miller avatar Aug 11 '22 05:08 Harris-Miller

@NotWoods @shellscape @NikolayMakhonin please see https://github.com/rollup/plugins/pull/1242

Harris-Miller avatar Aug 15 '22 03:08 Harris-Miller