search

jaredpalmer / tsdx

Zero-config CLI for TypeScript package development
https://tsdx.io
MIT License
11.22k stars 507 forks source link

When built with "importHelpers": true, tslib is not used #962

Closed RIP21 closed 3 years ago

RIP21 commented 3 years ago

Current Behavior

image

I build with importHelpers: true resulting bundles are still inlining helpers and aren't importing them from tslib.

Expected behavior

tslib helpers are imported where it's needed.

Suggested solution(s)

If it's not a bug and babel used to transpile the code and not tsc, then remove tslib as well as the importHelpers: true from templates and add @babel/runtime as a dependency. If it's a controversial/breaking change, add it as an optional setting.

Your environment

 System:
    OS: Linux 5.3 Ubuntu 20.04.1 LTS (Focal Fossa)
    CPU: (16) x64 AMD Ryzen 7 3700X 8-Core Processor
    Memory: 1.71 GB / 31.37 GB
    Container: Yes
    Shell: 5.8 - /usr/bin/zsh
  Binaries:
    Node: 14.15.4 - ~/.nvm/versions/node/v14.15.4/bin/node
    Yarn: 1.22.10 - ~/.nvm/versions/node/v14.15.4/bin/yarn
    npm: 6.14.10 - ~/.nvm/versions/node/v14.15.4/bin/npm
  Browsers:
    Chrome: 87.0.4280.88
    Firefox: 84.0.2
  npmPackages:
    tsdx: 0.14.1 => 0.14.1 
    typescript: 4.1.3 => 4.1.3
agilgur5 commented 3 years ago

If it's not a bug and babel used to transpile the code and not tsc

That's close to what TSDX does under-the-hood but rollup-plugin-typescript2 is used instead of tsc (both use the TS API) and actually both Babel and TS are used for transpilation. You can read more about this in https://github.com/formium/tsdx/issues/951#issuecomment-757527184, but to summarize we have TS -> ESNext -> Babel.

then remove tslib as well as the importHelpers: true from templates

So I didn't write that, but yes I would like to remove if it if possible. I had looked into it before and remember finding some edge cases, but can't remember all of them (I've probably detailed them in some issue or PR). Off the top of my head, it wasn't clear if other deps rely on this or if it's used when transpiling to ESNext too.

add @babel/runtime as a dependency.

The eventual plan I'd like to move to is to have authors include @babel/runtime as a dep, but there's a lot of work to get there. https://github.com/formium/tsdx/issues/968#issuecomment-791817207 goes over some of these issues for the use-case of polyfills, which is very similar. @babel/runtime actually has more complexity because @rollup/plugin-babel configures it.

If it's a controversial/breaking change, add it as an optional setting.

It would be quite breaking, but the automatic detection in the above linked comment would simplify that a lot. But that's a whole feature that needs to be built before this is really possible. Optional configuration is significantly easier said than done as well and creates flag sprawl and churn. It would actually make it harder and more time-consuming to get to the eventual goal, instead of less.

agilgur5 commented 3 years ago

I took a quick look again and found #72 and #73 that seem to have erroneously added tslib and importHelpers and they have simply existed since then. Babel had already been added at that point and TSDX was already doing TS -> ESNext -> Babel at that point, so it seems likely that was a mistake that failed to realize this.

In any case, would have to do a lot of testing to make sure it can be safely removed. It's existed for so long that things may even unintentionally work as a result of that mistake.

RIP21 commented 3 years ago

Yup, this is what I meant by "remove" as it's unused and I checked that in the code. Sorry that I didn't mentioned that, as I thought maintainers are well aware of all that.

My opinion what is the most optimal solution now IMO would be to ignore TSC emit JS completely, and use babel for everything TS->JS, esbuild as uglifier, and tsc only for declarations. Rollup to be used only as orchestrator of all these tools. Plus, you may use esbuild-jest instead of babel-jest/ts-jest, again, for speed. Type checking tests with tsc would be sufficient.

You can also compile with esbuild and apply optimizations only with babel, but that i never tested myself to recommend.

agilgur5 commented 3 years ago

Yup, this is what I meant by "remove" as it's unused and I checked that in the code. Sorry that I didn't mentioned that, as I thought maintainers are well aware of all that.

Well what I meant is that, it's not clear if it's never used in any builds, or if it just so happens that the builds we've observed never use it. I assume the latter is the majority case as well, and it's just edge cases where it might be used. Or maybe it is just none at all -- but don't want to break existing things without knowing for sure.

The old issues I linked to are from previous maintainers, so based on that and other linked issues, I'm fairly certain they were not aware of that. I've been bothered by it since I first noticed it, but haven't made the non-trivial effort to do all the testing to make sure it's ok to remove it (and add warnings that importHelpers etc are not supported). To give a sense, comments in the tsconfig template and updated template docs are really recent things I've added, but the templates in general don't get much attention.

The rest is pretty off-topic, but I'll respond a bit.

babel for everything TS->JS

Babel doesn't support various TS features or certain parts of the ecosystem, so that's not so straightforward. And it would be kind of disingenuous to claim being "TS-first" while not supporting those.

You can also compile with esbuild and apply optimizations only with babel, but that i never tested myself to recommend.

I was gonna say there's a whole issue on ESBuild already in https://github.com/formium/tsdx/issues/716 and that someone recently added an excellent write-up on some of the progress in the past year ...and then I saw that you were that person! 😅

But yea, as I wrote there, we wouldn't be able to support any Babel plugins that way. If we ran Babel afterword or at all that would defeat the purpose of optimizing with ESBuild. Even using Rollup eliminates a lot of the benefit because the difference is in orders of magnitude (as opposed to additive or even multiplicative). I'll respond more in that issue at some point (I was literally just re-reading it before you replied).

Plus, you may use esbuild-jest

Is that new? In your comment on the ESBuild issue you said Jest still wasn't supported. If you could add that there that would be great, would very much prefer to have all the info in one place