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

breaking(typescript): allow noEmit and emitDeclarationOnly #1206

Closed
wants to merge 0 commits into from

Conversation

Harris-Miller
Copy link
Contributor

@Harris-Miller Harris-Miller commented Jun 15, 2022

Rollup Plugin Name: typescript

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

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

Breaking Changes?

  • 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

@shellscape
Copy link
Collaborator

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

@NotWoods
Copy link
Member

NotWoods commented Jun 16, 2022

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.

@Harris-Miller
Copy link
Contributor Author

Harris-Miller commented Jun 17, 2022

@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?

@NotWoods
Copy link
Member

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

@Harris-Miller
Copy link
Contributor Author

@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

Copy link
Member

@NotWoods NotWoods left a comment

Choose a reason for hiding this comment

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

LGTM, just needs tests. Thank you for the contribution!

@NikolayMakhonin
Copy link

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

@shellscape
Copy link
Collaborator

@Harris-Miller please add some tests for this

@Harris-Miller
Copy link
Contributor Author

@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
Copy link
Contributor Author

Harris-Miller commented Aug 11, 2022

@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
Copy link
Contributor Author

@NotWoods @shellscape @NikolayMakhonin please see #1242

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.

None yet

4 participants