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

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

Closed
RIP21 opened this issue Jan 26, 2021 · 4 comments
Closed

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

RIP21 opened this issue Jan 26, 2021 · 4 comments
Labels
kind: feature New feature or request scope: templates Related to an init template, not necessarily to core (but could influence core) solution: intended behavior This is not a bug and is expected behavior

Comments

@RIP21
Copy link

RIP21 commented Jan 26, 2021

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 
@RIP21 RIP21 changed the title When built with "importHelpers": true tslib are still not used When built with "importHelpers": true tslib is not used Jan 26, 2021
@agilgur5 agilgur5 added kind: feature New feature or request solution: intended behavior This is not a bug and is expected behavior labels Mar 6, 2021
@agilgur5
Copy link
Collaborator

agilgur5 commented Mar 6, 2021

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 #951 (comment), 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. #968 (comment) 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 agilgur5 closed this as completed Mar 6, 2021
@agilgur5 agilgur5 added this to the Future Breaking milestone Mar 6, 2021
@agilgur5
Copy link
Collaborator

agilgur5 commented Mar 6, 2021

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

RIP21 commented Mar 6, 2021

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
Copy link
Collaborator

agilgur5 commented Mar 6, 2021

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 #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

@agilgur5 agilgur5 added the scope: templates Related to an init template, not necessarily to core (but could influence core) label Mar 6, 2021
@agilgur5 agilgur5 changed the title When built with "importHelpers": true tslib is not used When built with "importHelpers": true, tslib is not used Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: feature New feature or request scope: templates Related to an init template, not necessarily to core (but could influence core) solution: intended behavior This is not a bug and is expected behavior
Projects
None yet
Development

No branches or pull requests

2 participants