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
Conversation
Thanks for the PR. This is going to require approval from @NotWoods |
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:
If the code changes are complex enough its almost a separate plugin:
|
@NotWoods the changes are simple, so the
How does that sound? |
That sounds good to me. Please make sure to document 'none' with an example use case in the readme |
@NotWoods sorry for the delay, been very busy at work. I made the recommended changes by adding a Still need to add tests, will have that done in a week or so |
There was a problem hiding this 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!
This is a really necessary feature, because the |
@Harris-Miller please add some tests for this |
@shellscape @NikolayMakhonin @NotWoods |
@NotWoods I began writing tests and I found that the solution of adding a
Those changes essential change it so instead of forcing the Full
The more I look at this, I think just a simple 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 |
@NotWoods @shellscape @NikolayMakhonin please see #1242 |
Rollup Plugin Name:
typescript
This PR contains:
Are tests included?
Breaking Changes?
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
andemitDeclarationOnly
to befalse
. 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 foremitDeclarationOnly
rollup will simply load from the output of the previous plugin or from the file system. This is ideal to mimic the following:Where build uses babel to do all transpilation, including typescript via
@babel/preset-typescript
, andtsc
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 forcedemitDeclarationOnly: false
being on, all the code is run through thetypescript
transpiler first. If@rollup/plugin-babel
is set up to do totypescript
, there isn't a need to actually do thisBy 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 thegenerateBundle
step). On the large projects I tested this out at my work, I say initial builds drop from 20 seconds down to 2 secondsBreaking?
I would consider this a breaking change but it fundamentally changes the behavior of the plugin, any user that has
noEmit
oremitDeclarationOnly
set totrue
in theirtsconfig.json
for other purposes (egjest
) and are relying on the fact that those properties are forced tofalse
will see unintended change in their build. Therefor this has to be a Breaking changeTests
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